GROUP BY error when setting compress_segmentby with an enum column #4619

sb230132 · 2022-08-16T04:23:18Z

No description provided.

Enclose braces around empty if statement. Fixed build error on MacOS X.

codecov · 2022-08-16T04:32:42Z

Codecov Report

Merging #4619 (225d137) into main (03defb3) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #4619      +/-   ##
==========================================
- Coverage   90.77%   90.76%   -0.01%     
==========================================
  Files         224      224              
  Lines       41857    41975     +118     
==========================================
+ Hits        37994    38098     +104     
- Misses       3863     3877      +14

Impacted Files	Coverage Δ
tsl/src/nodes/decompress_chunk/decompress_chunk.c	`95.20% <80.00%> (-0.14%)`	⬇️
src/telemetry/functions.c	`91.33% <0.00%> (-2.98%)`	⬇️
src/cross_module_fn.c	`67.87% <0.00%> (-2.57%)`	⬇️
tsl/src/nodes/data_node_dispatch.c	`96.49% <0.00%> (-0.24%)`	⬇️
src/time_bucket.c	`98.25% <0.00%> (-0.16%)`	⬇️
tsl/src/bgw_policy/policies_v2.c	`96.27% <0.00%> (-0.12%)`	⬇️
tsl/src/bgw_policy/job.c	`88.62% <0.00%> (-0.05%)`	⬇️
src/chunk.c	`94.50% <0.00%> (-0.04%)`	⬇️
tsl/src/remote/dist_ddl.c	`95.82% <0.00%> (-0.02%)`	⬇️
src/chunk.h	`100.00% <0.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c5a759...225d137. Read the comment docs.

svenklemm

You should not change the vartype of the Var but instead fall back to anyenum lookup for the operators when the type is an enum and the lookup with the explicit type fails.

fix

fabriziomello · 2022-08-16T12:51:05Z

tsl/src/fdw/scan_plan.c

+	if (has_null_input)
+	{
+	}


Suggested change

if (has_null_input)

{

}

Fixed by removing the change.

I guess you forgot to remove it, no?

Right, i had this change in my main branch. Thus this change was re-appearing again and again. Reverted this change in main branch and fixed in current branch as well.

fabriziomello · 2022-08-16T13:00:09Z

Some comments:

You need provide a better commit message describing what is the problem you're solving and how you fixed it (for example Fix assertion in GRANT .. ON ALL TABLES IN SCHEMA #4582)
Remember squash commits before rebase it to the main branch, unless you really want to send several commits, and for that each one should have a good description and also use the Disable-check: commit-count in the PR description to silent the CI
Add to the thanks section in CHANGELOG the issue reporter

fabriziomello · 2022-08-17T21:25:29Z

tsl/src/nodes/decompress_chunk/decompress_chunk.c

+				else
+				{
+					elog(ERROR, "Invalid segmentby column");
+				}


Do we need this branch? If yes shouldn't have a test case for this?

Agree that its good to have a testcase. However iam not sure what test will hit this code.
I think it's better to have this code, so that it may hit in future. If we remove this, then code may further execute and report some unknown error or even worse case crash or return wrong results.

fabriziomello · 2022-08-17T21:28:25Z

Seems your code editor messed (automatically formatted) the CHANGELOG.md... we prefer to send coding format changes into a separated PR because it make useless the git blame. Check the .git-blame-ignore-revs in the root of source three.

svenklemm

I think we need to have similar check when we enable compression for the hypertable. No point having a compressed hypertable that errors when you try to query it.

svenklemm · 2022-08-17T23:02:42Z

tsl/src/nodes/decompress_chunk/decompress_chunk.c

+				}
+				else
+				{
+					elog(ERROR, "Invalid segmentby column");


This does not conform to pg error message style guide: https://www.postgresql.org/docs/current/error-style-guide.html
Should probably be more along the lines of sort operator lookup failed for column "%s"

Fixed as per your suggestion.

sb230132 · 2022-08-18T11:56:46Z

Seems your code editor messed (automatically formatted) the CHANGELOG.md... we prefer to send coding format changes into a separated PR because it make useless the git blame. Check the .git-blame-ignore-revs in the root of source three.

Right, its my editor which screwed up the formatting. Fixed now.

sb230132 · 2022-08-18T11:57:34Z

I think we need to have similar check when we enable compression for the hypertable. No point having a compressed hypertable that errors when you try to query it.

May be yes. But i dont have all the context required to check for these conditions during ALTER TABLE ... ENABLE COMPRESSION.

fabriziomello · 2022-08-18T19:24:21Z

tsl/src/nodes/decompress_chunk/decompress_chunk.c

+				}
+				else
+				{
+					elog(ERROR, "Sort operator lookup failed for column \"%s\"", column_name);


Suggested change

elog(ERROR, "Sort operator lookup failed for column \"%s\"", column_name);

elog(ERROR, "sort operator lookup failed for column \"%s\"", column_name);

Revert "Build error" This reverts commit e841fed. Fixed formatting in error message. Addressed review comments. Fixed formatting of CHANGELOG.md file. Addressed review comments. Updated error message as per Sven suggestion.

CHANGELOG.md

Co-authored-by: Sven Klemm <31455525+svenklemm@users.noreply.github.com>

sb230132

Fixed Sven comment.

sb230132 added 3 commits August 9, 2022 10:40

Build error

e841fed

Enclose braces around empty if statement. Fixed build error on MacOS X.

Merge branch 'timescale:main' into main

018a9f2

Merge branch 'timescale:main' into group-by-PR

876a210

sb230132 requested review from akuzm and gayyappan August 16, 2022 04:35

sb230132 marked this pull request as ready for review August 16, 2022 11:41

svenklemm requested changes Aug 16, 2022

View reviewed changes

mfundul reviewed Aug 16, 2022

View reviewed changes

fix Outdated Show resolved Hide resolved

fabriziomello reviewed Aug 16, 2022

View reviewed changes

sb230132 force-pushed the group-by-PR branch 5 times, most recently from 9c24007 to ba9950c Compare August 17, 2022 14:06

fabriziomello reviewed Aug 17, 2022

View reviewed changes

svenklemm reviewed Aug 17, 2022

View reviewed changes

sb230132 force-pushed the group-by-PR branch 2 times, most recently from 684064f to ba64a68 Compare August 18, 2022 12:10

fabriziomello assigned sb230132 Aug 18, 2022

fabriziomello reviewed Aug 18, 2022

View reviewed changes

Fixed grammar in error message.

f0c6c78

Revert "Build error" This reverts commit e841fed. Fixed formatting in error message. Addressed review comments. Fixed formatting of CHANGELOG.md file. Addressed review comments. Updated error message as per Sven suggestion.

sb230132 force-pushed the group-by-PR branch from ba64a68 to f0c6c78 Compare August 19, 2022 01:56

sb230132 requested review from fabriziomello and mfundul August 22, 2022 08:37

sb230132 requested a review from svenklemm August 22, 2022 08:48

svenklemm reviewed Aug 23, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

svenklemm approved these changes Aug 23, 2022

View reviewed changes

sb230132 added bug compression labels Aug 23, 2022

Update CHANGELOG.md

225d137

Co-authored-by: Sven Klemm <31455525+svenklemm@users.noreply.github.com>

sb230132 commented Aug 23, 2022

View reviewed changes

fabriziomello approved these changes Aug 25, 2022

View reviewed changes

sb230132 closed this Aug 29, 2022

sb230132 deleted the group-by-PR branch August 29, 2022 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GROUP BY error when setting compress_segmentby with an enum column #4619

GROUP BY error when setting compress_segmentby with an enum column #4619

sb230132 commented Aug 16, 2022

codecov bot commented Aug 16, 2022 •

edited

svenklemm left a comment

fabriziomello Aug 16, 2022

sb230132 Aug 18, 2022

fabriziomello Aug 18, 2022

sb230132 Aug 22, 2022

fabriziomello commented Aug 16, 2022

fabriziomello Aug 17, 2022

sb230132 Aug 18, 2022

fabriziomello commented Aug 17, 2022

svenklemm left a comment

svenklemm Aug 17, 2022

sb230132 Aug 18, 2022

sb230132 commented Aug 18, 2022

sb230132 commented Aug 18, 2022

fabriziomello Aug 18, 2022

sb230132 Aug 22, 2022

sb230132 left a comment

	elog(ERROR, "Sort operator lookup failed for column \"%s\"", column_name);
	elog(ERROR, "sort operator lookup failed for column \"%s\"", column_name);

GROUP BY error when setting compress_segmentby with an enum column #4619

GROUP BY error when setting compress_segmentby with an enum column #4619

Conversation

sb230132 commented Aug 16, 2022

codecov bot commented Aug 16, 2022 • edited

Codecov Report

svenklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziomello commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziomello commented Aug 17, 2022

svenklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb230132 commented Aug 18, 2022

sb230132 commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb230132 left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2022 •

edited