- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 263
GENERATE_SERIES function #8795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
GENERATE_SERIES function #8795
Conversation
| PSQL procedure vs built-in function performance testCREATE EXCEPTION E_INVALID_STEP 'Procedure SP_GENERATE_SERIES required values of parameter STEP_VALUE non equal zero';
SET TERM ^;
CREATE OR ALTER PROCEDURE SP_GENERATE_SERIES (
  START_VALUE  BIGINT,
  FINISH_VALUE BIGINT,
  STEP_VALUE   BIGINT DEFAULT 1)
RETURNS (
  CURRENT_VALUE BIGINT)
AS
BEGIN
  IF (STEP_VALUE = 0) THEN EXCEPTION E_INVALID_STEP;
  IF (STEP_VALUE > 0) THEN
    FOR CURRENT_VALUE = START_VALUE TO FINISH_VALUE BY STEP_VALUE DO
      SUSPEND;
  IF (STEP_VALUE < 0) THEN
    FOR CURRENT_VALUE = START_VALUE DOWNTO FINISH_VALUE BY ABS(STEP_VALUE) DO
      SUSPEND;
END^
SET TERM ;^1000000 records SELECT 
  COUNT(*), MIN(CURRENT_VALUE), MAX(CURRENT_VALUE)
FROM SP_GENERATE_SERIES(1, 1000000);SELECT 
  COUNT(*), MIN(N), MAX(N)
FROM GENERATE_SERIES(1, 1000000) S(N);10000000 records SELECT 
  COUNT(*), MIN(CURRENT_VALUE), MAX(CURRENT_VALUE)
FROM SP_GENERATE_SERIES(1, 10000000);SELECT 
  COUNT(*), MIN(N), MAX(N)
FROM GENERATE_SERIES(1, 10000000) S(N); | 
| It is a system selectable procedure with fixed number of parameters and output. Why did you create a new BLR verb instead of using existing BLR for calling it by name? | 
| 
 
 | 
| 
 This PR correctly extends  | 
| 
 It can be handled at DSQL stage. 
 Ugh, I see. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few little notes.
| ## Arguments | ||
|  | ||
| * `start` - The first value in the interval. `start` is specified as a variable, a literal, or a scalar expression of type | ||
| `smallint`, `integer`, `bigint` or `numeric(18, x)`. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about INT128?
| } | ||
| ; | ||
|  | ||
| %type <valueExprNode> step_opt | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call it comma_value_opt.
| DsqlDescMaker::fromNode(dsqlScratch, &stepDesc, stepItem, true); | ||
|  | ||
| // common scale | ||
| const auto scale = MIN(MIN(startDesc.dsc_scale, finishDesc.dsc_scale), stepDesc.dsc_scale); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's documented to be the max, but here you use min.
|  | ||
| if (!field) | ||
| { | ||
| const auto newField = FB_NEW_POOL(dsqlScratch->getPool()) dsql_fld(dsqlScratch->getPool()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use field directly without that extra newField.
| } | ||
| }; | ||
|  | ||
| class GenSeriesFunctionSourceNode : public TableValueFunctionSourceNode | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's better to declare the class as final instead of each method. Methods should be declared with override.
| class GenSeriesFunctionSourceNode : public TableValueFunctionSourceNode | ||
| { | ||
| public: | ||
| explicit GenSeriesFunctionSourceNode(MemoryPool& pool) : TableValueFunctionSourceNode(pool) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the common convention to put : ... in the same line as the constructor.
|  | ||
| class GenSeriesFunctionScan final : public TableValueFunctionScan | ||
| { | ||
| enum GenSeriesTypeItemIndex : unsigned | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why unsigned instead of UCHAR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't enum class a much more elegant thing than use common prefix for legacy enums?
| GenSeriesFunctionScan(CompilerScratch* csb, StreamType stream, const Firebird::string& alias, | ||
| ValueListNode* list); | ||
|  | ||
| protected: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use override in methods.
| } | ||
|  | ||
| // common scale | ||
| const auto scale = MIN(MIN(startDesc->dsc_scale, finishDesc->dsc_scale), stepDesc->dsc_scale); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIN vs MAX
|  | ||
| rpb->rpb_number.increment(); | ||
|  | ||
| do | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go to suggest better thing, but for me this code and m_recordExists is very confusing.
GENERATE_SERIES function
The
GENERATE_SERIESfunction creates a series of numbers within a specified interval.The interval and the step between series values are defined by the user.
Syntax
Arguments
start- The first value in the interval.startis specified as a variable, a literal, or a scalar expression of typesmallint,integer,bigintornumeric(18, x).finish- The last value in the interval.finishis specified as a variable, a literal, or a scalar expression oftype
smallint,integer,bigintornumeric(18, x). The series stops once the last generated step value exceedsthe
finishvalue.step- Indicates the number of values to increment or decrement between steps in the series.stepis an expressionof type
smallint,integer,bigintornumeric(18, x).stepcan be either negative or positive, but can't be zero (0). Thisargument is optional. The default value for
stepis 1.Returning type
The function
GENERATE_SERIESreturns a set withBIGINTorNUMERIC(18, x)column, where the scale isdetermined by the maximum of the scales of the function arguments.
Rules
If
start > finishand a negativestepvalue is specified, an empty set is returned.If
start < finishand a positivestepvalue is specified, an empty set is returned.If the
stepargument is zero, an error is thrown.Examples
A similar function exists in PostgreSQL: https://www.postgresql.org/docs/current/functions-srf.html and in MS SQL 2022: https://learn.microsoft.com/en-us/sql/t-sql/functions/generate-series-transact-sql?view=sql-server-ver17
PostgreSQL has the ability to generate date and time sequences, but we don't have interval types, so this isn't implemented. However, by generating a sequence of numbers, we can generate any sequence.