Skip to content

[CALCITE-6258] Map value constructor is unparsed incorrectly for PrestoSqlDialect#3683

Merged
macroguo-ghy merged 1 commit intoapache:mainfrom
YiwenWu:presto_dialect
Feb 21, 2024
Merged

[CALCITE-6258] Map value constructor is unparsed incorrectly for PrestoSqlDialect#3683
macroguo-ghy merged 1 commit intoapache:mainfrom
YiwenWu:presto_dialect

Conversation

@YiwenWu
Copy link
Contributor

@YiwenWu YiwenWu commented Feb 11, 2024

fixed Presto Map Function is unparsed incorrectly :map(array, array)
https://issues.apache.org/jira/browse/CALCITE-6258

@JiajunBernoulli
Copy link
Contributor

Good catch.
I have a similar PR: https://github.com/linkedin/coral/pull/99/files

final String expected = "SELECT MAP ('k1', 'v1', 'k2', 'v2')\n"
+ "FROM (VALUES (0)) t (ZERO)";
sql(query).withSpark().ok(expected);
final String expectedPresto = "SELECT MAP (ARRAY['k1', 'k2'], ARRAY['v1', 'v2'])\n"
Copy link
Member

@caicancai caicancai Feb 11, 2024

Choose a reason for hiding this comment

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

Hello, maybe we should add a comment with a jira link in the comment?

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, added new tests

} else {
// Current impl is same with Postgresql.
PostgresqlSqlDialect.DEFAULT.unparseCall(writer, call, leftPrec, rightPrec);
switch (call.getKind()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract them to createMapValueConstuctor function is bettter.

I like single responsibility principle

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, extract new function convertMapValueCall

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we extract more thoroughly?

We can use more parameter like unparseCall(writer, call, leftPrec, rightPrec)

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, extract map unparse method unparseMapValue

@YiwenWu YiwenWu force-pushed the presto_dialect branch 4 times, most recently from b6d27a9 to 582c94a Compare February 18, 2024 11:45
Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

+1 for this pr.

But MAP[](with no parameter) will be rejected by SqlMapValueConstructor#checkOperandTypes, so I think we need add a new SqlLibrary and add (or enable) map function for presto.

@macroguo-ghy
Copy link
Contributor

Please squash your commits, I will merge it later. @YiwenWu

@macroguo-ghy macroguo-ghy added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 21, 2024
@sonarqubecloud
Copy link

@macroguo-ghy macroguo-ghy merged commit 4369e88 into apache:main Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants