ebyhr commented on code in PR #12117:
URL: https://github.com/apache/iceberg/pull/12117#discussion_r1931401617
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/functions/SparkFunctions.java:
##########
@@ -30,14 +30,19 @@ public class SparkFunctions {
private SparkFunctions() {}
private static final Map<String, UnboundFunction> FUNCTIONS =
- ImmutableMap.of(
- "iceberg_version", new IcebergVersionFunction(),
- "years", new YearsFunction(),
- "months", new MonthsFunction(),
- "days", new DaysFunction(),
- "hours", new HoursFunction(),
- "bucket", new BucketFunction(),
- "truncate", new TruncateFunction());
+ new ImmutableMap.Builder<String, UnboundFunction>()
+ .put("iceberg_version", new IcebergVersionFunction())
+ .put("years", new YearsFunction())
+ .put("year", new YearsFunction())
+ .put("months", new MonthsFunction())
+ .put("month", new MonthsFunction())
+ .put("days", new DaysFunction())
+ .put("day", new DaysFunction())
+ .put("hours", new HoursFunction())
+ .put("hour", new HoursFunction())
Review Comment:
These function classes contain examples with plural styles in javadoc. Can
we add another example or just update them?
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/functions/SparkFunctions.java:
##########
@@ -30,14 +30,19 @@ public class SparkFunctions {
private SparkFunctions() {}
private static final Map<String, UnboundFunction> FUNCTIONS =
- ImmutableMap.of(
- "iceberg_version", new IcebergVersionFunction(),
- "years", new YearsFunction(),
- "months", new MonthsFunction(),
- "days", new DaysFunction(),
- "hours", new HoursFunction(),
- "bucket", new BucketFunction(),
- "truncate", new TruncateFunction());
+ new ImmutableMap.Builder<String, UnboundFunction>()
+ .put("iceberg_version", new IcebergVersionFunction())
+ .put("years", new YearsFunction())
+ .put("year", new YearsFunction())
Review Comment:
We could extract a constant instead of initializing two instances.
(This comment might be conflicted with my another comment in
TestSparkDaysFunction)
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkDaysFunction.java:
##########
@@ -39,6 +39,9 @@ public void testDates() {
assertThat(scalarSql("SELECT system.days(date('2017-12-01'))"))
.as("Expected to produce 2017-12-01")
.isEqualTo(Date.valueOf("2017-12-01"));
+ assertThat(scalarSql("SELECT system.day(date('2017-12-01'))"))
Review Comment:
Can we also update negative test cases? e.g. testWrongNumberOfArguments
The function name is hard-coded in each function class in `name()` and
`canonicalName()` methods. We may want to add a constructor taking the function
name to report the correct function 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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
