Thread: Re: Added prosupport function for estimating numeric generate_series rows

On Fri, 29 Nov 2024 at 06:25, 孤傲小二~阿沐 <tsinghualucky912@foxmail.com> wrote:
> Hello, thank you very much for your attention and guidance. I have modified and improved the problem you mentioned.
Thepatch of version v2 is attached below. 

I've only had a quick look at the patch.

* The following needs a DatumGetFloat8():

+ req->rows = numericvar_to_double_no_overflow(&q) + 1;

* It would also be good to see a comment explaining the following line:

+ if(nstep.sign != var_diff.sign)

Something like: /* When the sign of the step size and the series range
don't match, there are no rows in the series. */

* You should add one test for the generate_series(numeric, numeric).
You've only got tests for the 3 arg version.

Also a few minor things:

* Missing space after "if"

+ if(arg3)

* We always have an empty line after variable declarations, that's missing in:

+ NumericVar q;
+ init_var(&q);

* No need for the braces in:

+ if (NUMERIC_IS_SPECIAL(step))
+ {
+ goto cleanup;
+ }

David

David Rowley <dgrowleyml@gmail.com> writes:
> Also a few minor things:
> * Missing space after "if"
> + if(arg3)
> * We always have an empty line after variable declarations, that's missing in:
> + NumericVar q;
> + init_var(&q);

This sort of stuff is best addressed by running the code through
pgindent, rather than fixing it manually.  Usually we don't insist
on submitters getting it right; the committer should pgindent it.

            regards, tom lane

On Fri, 29 Nov 2024 at 18:50, songjinzhou <tsinghualucky912@foxmail.com> wrote:
> Hello, thank you and David Rowley for your comments.
>
> I have used pgindent to adjust the code format and added comments and missing regression test cases. Here is the
patchof version v3.
 

It looks fine to me.  The only things I'd adjust are stylistic,
namely; 1) remove two tabs before the goto label, 2) remove redundant
braces around the goto cleanup, 3) rename the variable "q" to
something slightly more meaningful, maybe "res" or "rows".

I'll defer to Dean.

David