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-6771] Convert Type from FLOAT to DOUBLE in PrestoDialect #4132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xiaochen-zhou
Copy link
Contributor

Since FLOAT type is not supported in Presto

According to the documentation: https://prestodb.io/docs/current/language/types.html#real
image
Using CAST(xxxx AS FLOAT) will result in an error:


SELECT CAST(message_count AS FLOAT)

FROM applydata_bigdata.kafka_topic_cluster_user_dept_month_one_day_msg_info

WHERE ymd = 20250105

image
It should be converted to the DOUBLE type instead.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jan 9, 2025
@xiaochen-zhou xiaochen-zhou force-pushed the presto_float_double branch 2 times, most recently from b367390 to fba35ad Compare January 9, 2025 11:02
@@ -9093,6 +9093,20 @@ private void checkLiteral2(String expression, String expected) {
sql(query).withOracle().ok(oracle);
}

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-6771">[CALCITE-6771]
* Convert Type from FLOAT to DOUBLE in PrestoDialect </a>. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: No space in PrestoDialect and </a>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor: No space in PrestoDialect and </a>;

Done.

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