You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/07/13 07:05:34 UTC
[GitHub] [calcite] vnhive opened a new pull request #2459: CALCITE-4692 : Calcite typecasts the output of aggregate functions on integer columns to DOUBLE
vnhive opened a new pull request #2459:
URL: https://github.com/apache/calcite/pull/2459
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak closed pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2459:
URL: https://github.com/apache/calcite/pull/2459
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] NobiGo commented on pull request #2459: CALCITE-4692 : Calcite typecasts the output of aggregate functions on integer columns to DOUBLE
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#issuecomment-878857142
Please reformat the PR commit message: [CALCITE-XXXX] XXXXXXXX(Name)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#discussion_r669563116
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -5430,6 +5430,43 @@ private void checkLiteral2(String expression, String expected) {
.ok(expected);
}
+
+ @Test void testTINYINTRedshift() {
Review comment:
Reserved words are case insensitive so TINYINT vs. tinyint does not make a different for Calcite. Please remove this variant.
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -5430,6 +5430,43 @@ private void checkLiteral2(String expression, String expected) {
.ok(expected);
}
+
+ @Test void testTINYINTRedshift() {
+ String query = "SELECT CAST(\"department_id\" AS TINYINT) FROM \"employee\"";
+ String expected = "SELECT CAST(\"department_id\" AS \"int2\")\n"
+ + "FROM \"foodmart\".\"employee\"";
+ sql(query)
+ .withRedshift()
+ .ok(expected);
+ }
+
+ @Test void testtinyintRedshift() {
Review comment:
To keep things uniform with existing tests in the class rename to:
`testRedshiftCastToTinyint` (see `testMysqlCastToBigint` for instance). You could also name it `testCastToTinyint`, leaving it a bit more general (similar to `testCastToVarchar`), which could be extended later for different dialects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vnhive commented on a change in pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
vnhive commented on a change in pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#discussion_r669517866
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
Review comment:
Done. Found smallint (int2) and double precision (float8) that required attention. Fixed them for now in this patch and wrote the necessary test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2459: CALCITE-4692 : Calcite typecasts the output of aggregate functions on integer columns to DOUBLE
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#discussion_r668503419
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
Review comment:
Since you are working on this It might worth checking if the rest of the types (at least the most common) in the `SqlTypeName` have an alternative for Redshift so that you fix everything in one-shot.
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
+ case TINYINT:
+ // Postgres has no tinyint (1 byte), so instead cast to smallint (2 bytes)
Review comment:
Typo: Postgres -> Redshift
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
+ case TINYINT:
+ // Postgres has no tinyint (1 byte), so instead cast to smallint (2 bytes)
+ castSpec = "smallint";
+ break;
+ case DOUBLE:
+ // Postgres has a double type but it is named differently
Review comment:
Typo: Postgres -> Redshift
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] NobiGo commented on pull request #2459: CALCITE-4692 : Calcite typecasts the output of aggregate functions on integer columns to DOUBLE
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#issuecomment-878859925
Please add RedShift dialect info in PR comment message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vnhive commented on a change in pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
vnhive commented on a change in pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#discussion_r669517049
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
+ case TINYINT:
+ // Postgres has no tinyint (1 byte), so instead cast to smallint (2 bytes)
Review comment:
Done.
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
##########
@@ -45,4 +49,24 @@ public RedshiftSqlDialect(Context context) {
@Nullable SqlNode fetch) {
unparseFetchUsingLimit(writer, offset, fetch);
}
+
+ @Override public SqlNode getCastSpec(RelDataType type) {
+ String castSpec;
+ switch (type.getSqlTypeName()) {
+ case TINYINT:
+ // Postgres has no tinyint (1 byte), so instead cast to smallint (2 bytes)
+ castSpec = "smallint";
+ break;
+ case DOUBLE:
+ // Postgres has a double type but it is named differently
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vnhive commented on pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
vnhive commented on pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#issuecomment-880742434
> Please also resolve merge conflicts in order for the CI to run.
Done !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vnhive commented on a change in pull request #2459: CALCITE-4692 : [Redshift Dialect] Redshift does not support the DOUBLE datatype
Posted by GitBox <gi...@apache.org>.
vnhive commented on a change in pull request #2459:
URL: https://github.com/apache/calcite/pull/2459#discussion_r670515445
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -5430,6 +5430,43 @@ private void checkLiteral2(String expression, String expected) {
.ok(expected);
}
+
+ @Test void testTINYINTRedshift() {
Review comment:
Done !
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -5430,6 +5430,43 @@ private void checkLiteral2(String expression, String expected) {
.ok(expected);
}
+
+ @Test void testTINYINTRedshift() {
+ String query = "SELECT CAST(\"department_id\" AS TINYINT) FROM \"employee\"";
+ String expected = "SELECT CAST(\"department_id\" AS \"int2\")\n"
+ + "FROM \"foodmart\".\"employee\"";
+ sql(query)
+ .withRedshift()
+ .ok(expected);
+ }
+
+ @Test void testtinyintRedshift() {
Review comment:
Done !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org