apilloud commented on a change in pull request #12643:
URL: https://github.com/apache/beam/pull/12643#discussion_r489084327
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -87,16 +87,17 @@
.put("$extract_time", "TIME")
.put("$extract_datetime", "DATETIME")
.build();
- public static final String DOUBLE_POSITIVE_INF_FUNCTION =
"double_positive_inf";
- public static final String DOUBLE_NEGATIVE_INF_FUNCTION =
"double_negative_inf";
- public static final String DOUBLE_NAN_FUNCTION = "double_nan";
- private static final Map<String, String> DOUBLE_FUNCTIONS =
+ public static final String DOUBLE_POSITIVE_INF_WRAPPER =
"double_positive_inf";
Review comment:
nit: Renames like this are probably a net negative when you take into
account the total cost of a change (time authoring and reviewing, increased
diffs when debugging future issues, potential upgrade friction for customers).
It is hard to know exactly where the line is so feel free to push forward with
these as much of the work is already done. In the future I would suggest
avoiding non-functional changes, particularly if they are something that would
be at most a nit on a code review. (These went in recently and didn't even get
a nit comment.)
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -475,17 +512,22 @@
// FunctionSignatureId.FN_RANGE_BUCKET, // range_bucket(T,
array<T>) -> int64
- // FunctionSignatureId.FN_RAND, // rand() -> double
+ FunctionSignatureId.FN_RAND // rand() -> double
Review comment:
This one scares me just a little. In particular, I'm worried that we
might accidentally optimize such that we end up with a constant. Is that
something we need to test? https://xkcd.com/221/
##########
File path:
sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlTypesUtils.java
##########
@@ -24,6 +24,11 @@
@Internal
public class ZetaSqlTypesUtils {
+ public static final BigDecimal NUMERIC_MAX_VALUE =
Review comment:
nit: these constants are only used in `ZetaSqlMathFunctionsTest.java`,
do they belong there?
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -277,29 +293,31 @@
FunctionSignatureId.FN_PARSE_DATETIME, // parse_datetime
FunctionSignatureId.FN_PARSE_TIME, // parse_time
FunctionSignatureId.FN_PARSE_TIMESTAMP, // parse_timestamp
+ // FunctionSignatureId.FN_LAST_DAY_DATE, // last_day date
+ // FunctionSignatureId.FN_LAST_DAY_DATETIME, // last_day datetime
// Math functions
- // FunctionSignatureId.FN_ABS_INT64, // abs
- // FunctionSignatureId.FN_ABS_DOUBLE, // abs
- // FunctionSignatureId.FN_ABS_NUMERIC, // abs
+ FunctionSignatureId.FN_ABS_INT64, // abs
Review comment:
nit: This PR has a interesting mix of minor refactors and addition of
new functionality. These should probably be separate changes.
----------------------------------------------------------------
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:
[email protected]
