apilloud commented on a change in pull request #12174:
URL: https://github.com/apache/beam/pull/12174#discussion_r454615048
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -156,7 +156,11 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (FUNCTIONS_USING_INTERVAL.contains(funName)) {
+ if (funName.equals("numeric_literal")) {
Review comment:
Reverse the order here (`"numeric_literal".equals(funName)`) to avoid
potential issues if funName is null. Also consider making "numeric_literal" a
constant like the others.
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlTypesUtils.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.zetasql;
+
+import java.math.BigDecimal;
+import org.apache.beam.sdk.annotations.Internal;
+
+/** Utils to deal with ZetaSQL type generation. */
+@Internal
+public class ZetaSqlTypesUtils {
Review comment:
This class looks to be for tests only. Move it to '/src/test/' instead
of '/src/main/'?
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -805,6 +807,24 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind,
Value value) {
.makeLiteral(
value.getStringValue(),
typeFactory().createSqlType(SqlTypeName.VARCHAR), true);
break;
+ case TYPE_NUMERIC:
+ // Cannot simply call makeExactLiteral() for ZetaSQL NUMERIC type
because later it will be
+ // unparsed to the string representation of the BigDecimal itself
(e.g. "SELECT NUMERIC '0'"
+ // will be unparsed to "SELECT 0E-9"), and Calcite does not allow
customize unparsing of
+ // SqlNumericLiteral. So we create a wrapper function here such that
we can later recognize
+ // it and customize its unparsing in BeamBigQuerySqlDialect.
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createSimpleSqlFunction(
+ "numeric_literal",
ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)),
Review comment:
I've seen this constant `numeric_literal` before somewhere...
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -805,6 +807,24 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind,
Value value) {
.makeLiteral(
value.getStringValue(),
typeFactory().createSqlType(SqlTypeName.VARCHAR), true);
break;
+ case TYPE_NUMERIC:
+ // Cannot simply call makeExactLiteral() for ZetaSQL NUMERIC type
because later it will be
Review comment:
This is a somewhat interesting approach. It might be worth writing up a
mini design doc on why this approach was chosen and literals didn't work (but
ask @robinyqiu first). I suspect this could also fix the Infinity and NaN
issues I told you about (which are actually in Double, not Numeric BEAM-8057).
----------------------------------------------------------------
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:
us...@infra.apache.org
