-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[CALCITE-6220] Rewrite MIN/MAX(BOOLEAN) as BOOL_AND/BOOL_OR for Postgres, Redshift #3646
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
Conversation
905ec7e
to
28036af
Compare
e6bd8f7
to
d920051
Compare
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 requested changes.
Also, in the jira summary, change MIN/MAX(bool)
to MIN/MAX(BOOLEAN)
. bool
is not a SQL data type.
@@ -872,6 +872,13 @@ public SqlNode rewriteSingleValueExpr(SqlNode aggCall, RelDataType relDataType) | |||
return aggCall; | |||
} | |||
|
|||
/** With x as BOOLEAN column, rewrite MAX(x)/MIN(x) as BOOL_OR(x)/BOOL_AND(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.
Say what the method does. Nothing to do with BOOLEAN. Declarative "Rewrites" not imperative "rewrite".
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.
Done
default: | ||
super.unparseCall(writer, call, leftPrec, rightPrec); | ||
} | ||
} | ||
|
||
@Override public SqlNode rewriteMaxMinExpr(SqlNode aggCall, RelDataType relDataType) { |
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.
Don't instantatiate a dialect. Unnecessary coupling. Use a helper method. E.g. static void unparseFetchUsingLimit()
.
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.
Done
+ "MIN(\"brand_name\")\n" | ||
+ "FROM \"foodmart\".\"product\""; | ||
sql(query).ok(expected); | ||
sql(query).withPostgresql().ok(expectedPostgres); |
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 would chain. And add a test for a 'neutral' dialect:
sql(query).ok(expected)
.withPostgresql().ok(expectedPostgres)
.withRedshift().ok(expectedRedshift)
.withMySQL().ok(expected);
I like how you tested on a non-BOOLEAN column
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.
Done
fcc104b
to
2f2c0ad
Compare
553184f
to
6d3302b
Compare
|
No description provided.