Skip to content

[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

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

tanclary
Copy link
Contributor

No description provided.

@tanclary tanclary requested a review from julianhyde January 24, 2024 20:02
@tanclary tanclary force-pushed the 6220-booland branch 2 times, most recently from e6bd8f7 to d920051 Compare January 24, 2024 20:56
Copy link
Contributor

@julianhyde julianhyde left a 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)
Copy link
Contributor

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".

Copy link
Contributor Author

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) {
Copy link
Contributor

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().

Copy link
Contributor Author

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);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tanclary tanclary changed the title [CALCITE-6220] Rewrite MIN/MAX(bool) as BOOL_AND/BOOL_OR for Postgres, Redshift [CALCITE-6220] Rewrite MIN/MAX(BOOLEAN) as BOOL_AND/BOOL_OR for Postgres, Redshift Jan 24, 2024
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
100.0% Coverage on New Code
2.3% Duplication on New Code

See analysis details on SonarCloud

@tanclary tanclary merged commit 26b5e9b into apache:main Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants