Skip to content
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

[CALCITE-6796] Convert Type from BINARY to VARBINARY in PrestoDialect #4167

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

xiaochen-zhou
Copy link
Contributor

Since BINARY type is not supported in Presto

According to the documentation:

https://prestodb.io/docs/current/language/types.html#varbinary

Using CAST(xxxx AS BINARY) will result in an error:

SELECT cast(task_id as binary)
from applydata_bigdata.xflink_task_info
where reader_info like '%mongodbreader%' or writer_info like '%mongodbwriter%';
image

It should be converted to the VARBINARY type instead.

Copy link
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

overall LGTM

@@ -153,6 +153,9 @@ private static void unparseUsingLimit(SqlWriter writer, @Nullable SqlNode offset
case FLOAT:
return new SqlDataTypeSpec(
new SqlBasicTypeNameSpec(SqlTypeName.DOUBLE, SqlParserPos.ZERO), SqlParserPos.ZERO);
case BINARY:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add the link to VARBINARY for presto, as presto does not support varbinary(n) and this may cause confusion

@caicancai caicancai added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jan 25, 2025
@xiaochen-zhou xiaochen-zhou force-pushed the presto_binary branch 9 times, most recently from 0f77a0a to a083d09 Compare January 26, 2025 01:38
@caicancai caicancai added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed LGTM-will-merge-soon Overall PR looks OK. Only minor things left. labels Jan 26, 2025
@caicancai caicancai merged commit c79d6d4 into apache:main Jan 26, 2025
17 of 18 checks passed
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.

2 participants