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 2020/01/02 07:55:29 UTC
[GitHub] [calcite] dhirenda-gautam opened a new pull request #1714: [Calcite
3663] Support for TRIM function in BigQuery dialect
dhirenda-gautam opened a new pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714
In current Calcite implementation for query : SELECT TRIM('ABC') for big-query Dialect it translated into SELECT TRIM(BOTH ' ' FROM 'ABC') .
But the appropriate query for BigQuery is :: SELECT TRIM('ABC')
Similarly below query
SELECT TRIM(BOTH 'a' from 'ABC')
SELECT TRIM(LEADING ' ' from 'ABC')
SELECT TRIM(TRAILING ' ' from 'ABC') are translated into invalid BigQuery query.
Unparse logic for the trim has been handle in BigQuery dialect to convert the source Trim query into valid bigQuery query.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836861
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -958,36 +958,82 @@ private static String toSql(RelNode root, SqlDialect dialect) {
* <a href="https://issues.apache.org/jira/browse/CALCITE-3220">[CALCITE-3220]
* HiveSqlDialect should transform the SQL-standard TRIM function to TRIM,
* LTRIM or RTRIM</a>. */
- @Test public void testHiveTrim() {
+ @Test public void testHiveAndBqTrim() {
Review comment:
Added the comment for the BQ test cases along with JIRA Id
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1714:
[Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362580437
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -958,36 +958,82 @@ private static String toSql(RelNode root, SqlDialect dialect) {
* <a href="https://issues.apache.org/jira/browse/CALCITE-3220">[CALCITE-3220]
* HiveSqlDialect should transform the SQL-standard TRIM function to TRIM,
* LTRIM or RTRIM</a>. */
- @Test public void testHiveTrim() {
+ @Test public void testHiveAndBqTrim() {
Review comment:
As you can see there is a comment about this test case which is hive related. So you can also add comments for BQ.
You can either add another comment for BQ, or split this test case to hive and BQ and then add comments seperately.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on issue #1714: [Calcite 3663]
Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
chunweilei commented on issue #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#issuecomment-571410172
@dhirenda-gautam Please modify the commit message to the right format, namely
`[CALCITE-3663] Support for TRIM function in BigQuery dialect`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1714:
[Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362579227
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
Review comment:
Also need to validate the second operand?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on issue #1714: [Calcite 3663]
Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on issue #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#issuecomment-571099092
Rebase the branch and solved conflict
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836648
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -958,36 +958,82 @@ private static String toSql(RelNode root, SqlDialect dialect) {
* <a href="https://issues.apache.org/jira/browse/CALCITE-3220">[CALCITE-3220]
* HiveSqlDialect should transform the SQL-standard TRIM function to TRIM,
* LTRIM or RTRIM</a>. */
- @Test public void testHiveTrim() {
+ @Test public void testHiveAndBqTrim() {
Review comment:
@amaliujia I have remove this check as this is an unnessary check over here because in SqlTrimFunction class checkOperandTypes method perform the Operands check.
Removed this assert as the operands type check is already done in SqlTrimFunction class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on issue #1714: [Calcite 3663]
Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on issue #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#issuecomment-571444952
@chunweilei thanks for suggestion i have update the commit 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362846450
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
Review comment:
Removed this assert as the operands type check is already done in SqlTrimFunction class
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836762
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
Review comment:
@amaliujia I have remove this check as this is an unnessary check over here because in SqlTrimFunction class checkOperandTypes method perform the Operands check.
Removed this assert as the operands type check is already done in SqlTrimFunction class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836762
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
Review comment:
@amaliujia I have remove this check as this is an unnessary check over here because in SqlTrimFunction class checkOperandTypes method perform the Operands check.
Removed this assert as the operands type check is already done in SqlTrimFunction class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1714:
[Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362699302
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
+ final String operatorName;
+ SqlLiteral trimFlag = call.operand(0);
+ SqlLiteral valueToTrim = call.operand(1);
+ switch (trimFlag.getValueAs(SqlTrimFunction.Flag.class)) {
+ case LEADING:
+ operatorName = "LTRIM";
+ break;
+ case TRAILING:
+ operatorName = "RTRIM";
+ break;
+ default:
+ operatorName = call.getOperator().getName();
+ break;
+ }
+ final SqlWriter.Frame trimFrame = writer.startFunCall(operatorName);
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ if (!valueToTrim.toValue().matches("\\s+")) {
Review comment:
This `if `logic is different with that of `HiveSqlDialect`. Could you add doc for it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836648
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -958,36 +958,82 @@ private static String toSql(RelNode root, SqlDialect dialect) {
* <a href="https://issues.apache.org/jira/browse/CALCITE-3220">[CALCITE-3220]
* HiveSqlDialect should transform the SQL-standard TRIM function to TRIM,
* LTRIM or RTRIM</a>. */
- @Test public void testHiveTrim() {
+ @Test public void testHiveAndBqTrim() {
Review comment:
@amaliujia I have remove this check as this is an unnessary check over here because in SqlTrimFunction class checkOperandTypes method perform the Operands check.
Removed this assert as the operands type check is already done in SqlTrimFunction class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam commented on a change in pull request
#1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam commented on a change in pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#discussion_r362836917
##########
File path: core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java
##########
@@ -192,6 +197,37 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
}
}
+ /**
+ * For usage of TRIM, LTRIM and RTRIM in BQ see
+ * <a href="https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#trim">
+ * BQ Trim Function</a>.
+ */
+ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec,
+ int rightPrec) {
+ assert call.operand(0) instanceof SqlLiteral : call.operand(0);
+ final String operatorName;
+ SqlLiteral trimFlag = call.operand(0);
+ SqlLiteral valueToTrim = call.operand(1);
+ switch (trimFlag.getValueAs(SqlTrimFunction.Flag.class)) {
+ case LEADING:
+ operatorName = "LTRIM";
+ break;
+ case TRAILING:
+ operatorName = "RTRIM";
+ break;
+ default:
+ operatorName = call.getOperator().getName();
+ break;
+ }
+ final SqlWriter.Frame trimFrame = writer.startFunCall(operatorName);
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ if (!valueToTrim.toValue().matches("\\s+")) {
Review comment:
Added the doc which specify the purpose of the if block
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei merged pull request #1714: [Calcite 3663]
Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] dhirenda-gautam edited a comment on issue #1714: [Calcite
3663] Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
dhirenda-gautam edited a comment on issue #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#issuecomment-571444952
@chunweilei thanks for suggestion i have updated the commit 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on issue #1714: [Calcite 3663]
Support for TRIM function in BigQuery dialect
Posted by GitBox <gi...@apache.org>.
amaliujia commented on issue #1714: [Calcite 3663] Support for TRIM function in BigQuery dialect
URL: https://github.com/apache/calcite/pull/1714#issuecomment-570713589
LGTM thanks.
It could be better if you can have a rebase locally and solve conflicts so people can easy merge your PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services