You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "clettieri (via GitHub)" <gi...@apache.org> on 2023/07/31 18:40:18 UTC
[GitHub] [iceberg] clettieri opened a new pull request, #8192: Docs: Correct partition transform functions on spec.md
clettieri opened a new pull request, #8192:
URL: https://github.com/apache/iceberg/pull/8192
Given the transform functions listed [here](https://github.com/apache/iceberg/blob/84e526f24140ee77d412e1a4b2736fc0c93a070f/spark/v3.1/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java#L413), the docs now reflect the correct transform function names.
Hopefully this saves someone else a few cycles of debugging :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1732443128
Thank you, @clettieri!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1663019099
I would want @rdblue to check this. If I remember correctly we originally did this because the base implementation happened to be wrong so we had to add another implementation that took account for 0 correctly.
The other issue here is that the non-plural names are Spark native functions, is that an issue?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1732154019
Looks like there are some spotless issues that fail the build.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280667776
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
I added singular options to each Spark3 version. How does this look?
I can add some tests as well if you think that would be useful.
Also, should I change the prefix of this PR to `Spark:` ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1726354240
@clettieri, sorry it took so long to get back to this PR.
Could you, please, add tests that the new name is also supported in CREATE TABLE statements too? I believe this change only covers ALTER TABLE. In addition, this PR should be rebased and include Spark 3.5.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280530315
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
I can do that.
Would it be useful to keep the unintended "backward" compatibility though? Or do you think just force the Spark API to match the spec?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280667776
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
I added singular options to each Spark3 version. How does this look?
I can add some tests as well if you think that would be useful.
Also, should I change the prefix of this PR to `:Spark` ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280743485
##########
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -342,14 +342,18 @@ public static Term toIcebergTerm(Expression expr) {
return org.apache.iceberg.expressions.Expressions.ref(colName);
case "bucket":
return org.apache.iceberg.expressions.Expressions.bucket(colName, findWidth(transform));
+ case "year":
case "years":
return org.apache.iceberg.expressions.Expressions.year(colName);
+ case "month"
Review Comment:
```suggestion
case "month":
```
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java:
##########
@@ -366,14 +366,18 @@ public static Term toIcebergTerm(Expression expr) {
return org.apache.iceberg.expressions.Expressions.ref(colName);
case "bucket":
return org.apache.iceberg.expressions.Expressions.bucket(colName, findWidth(transform));
+ case "year"
Review Comment:
```suggestion
case "year":
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280106568
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
cc: @aokolnychyi, @RussellSpitzer
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1728162582
> @clettieri, sorry it took so long to get back to this PR.
>
> Could you, please, add tests that the new name is also supported in CREATE TABLE statements too? I believe this change only covers ALTER TABLE. In addition, this PR should be rebased and include Spark 3.5.
Hey @aokolnychyi, I can rebase and include Spark 3.5 👍🏼 .
Regarding the CREATE TABLE tests though, I don't see any for the previous transform functions and am unclear what new functionality they would test compared to what we have now. The current tests seem sufficient IMO to validate the transform functions can be called. Am I missing something here? Should I create a new test class to very creating tables with these transform functions succeed?
Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1733415633
> I fixed a typo and opened [clettieri#1](https://github.com/clettieri/iceberg/pull/1) with the spotless changes.
Thank you for saving me the time this morning :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1733659268
thanks @clettieri
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1324506388
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java:
##########
@@ -179,6 +179,82 @@ public void testAddHoursPartition() {
Assert.assertEquals("Should have new spec field", expected, table.spec());
}
+ @Test
+ public void testAddYearPartition() {
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, category string, ts timestamp, data string) USING iceberg",
+ tableName);
+ Table table = validationCatalog.loadTable(tableIdent);
+
+ Assert.assertTrue("Table should start unpartitioned", table.spec().isUnpartitioned());
+
+ sql("ALTER TABLE %s ADD PARTITION FIELD year(ts)", tableName);
+
+ table.refresh();
+
+ PartitionSpec expected =
+ PartitionSpec.builderFor(table.schema()).withSpecId(1).year("ts").build();
+
+ Assert.assertEquals("Should have new spec field", expected, table.spec());
Review Comment:
Will do :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1732664410
I fixed a typo and opened https://github.com/clettieri/iceberg/pull/1 with the spotless 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra merged pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #8192:
URL: https://github.com/apache/iceberg/pull/8192
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1324918825
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java:
##########
@@ -179,6 +179,82 @@ public void testAddHoursPartition() {
Assert.assertEquals("Should have new spec field", expected, table.spec());
}
+ @Test
+ public void testAddYearPartition() {
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, category string, ts timestamp, data string) USING iceberg",
+ tableName);
+ Table table = validationCatalog.loadTable(tableIdent);
+
+ Assert.assertTrue("Table should start unpartitioned", table.spec().isUnpartitioned());
+
+ sql("ALTER TABLE %s ADD PARTITION FIELD year(ts)", tableName);
+
+ table.refresh();
+
+ PartitionSpec expected =
+ PartitionSpec.builderFor(table.schema()).withSpecId(1).year("ts").build();
+
+ Assert.assertEquals("Should have new spec field", expected, table.spec());
Review Comment:
@nastra updated the tests, let me know what you think
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1324009028
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java:
##########
@@ -179,6 +179,82 @@ public void testAddHoursPartition() {
Assert.assertEquals("Should have new spec field", expected, table.spec());
}
+ @Test
+ public void testAddYearPartition() {
+ sql(
+ "CREATE TABLE %s (id bigint NOT NULL, category string, ts timestamp, data string) USING iceberg",
+ tableName);
+ Table table = validationCatalog.loadTable(tableIdent);
+
+ Assert.assertTrue("Table should start unpartitioned", table.spec().isUnpartitioned());
+
+ sql("ALTER TABLE %s ADD PARTITION FIELD year(ts)", tableName);
+
+ table.refresh();
+
+ PartitionSpec expected =
+ PartitionSpec.builderFor(table.schema()).withSpecId(1).year("ts").build();
+
+ Assert.assertEquals("Should have new spec field", expected, table.spec());
Review Comment:
it would be great to not add more JUnit4-style assertions (even if the underlying class already uses them). Can you please convert those to AssertJ? That will make migration to JUnit5 easier. See https://iceberg.apache.org/contribute/#testing for some details
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1733732330
Thanks for fixing. I couldn't take a look at it again.
LGTM.
But we missed to update the documentation in https://github.com/apache/iceberg/blob/master/docs/spark-ddl.md.
@nk1506 would you like to work on 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280106323
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
This might impact Trino, Dremio and other engines which has naming conventions according to spec.
https://trino.io/docs/current/connector/iceberg.html#partitioned-tables
https://docs.dremio.com/cloud/reference/sql/commands/create-table/
IMO, better to change the spark transform name and deprecate the old ones.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280667776
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
I added singular options to each Spark3 version. How does this look?
I can add some tests as well if you think that would be useful.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280742773
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
> I can add some tests as well if you think that would be useful.
You can add a test in `TestAlterTablePartitionFields` to make sure that we don't break this in the future. It could be something similar as `testSparkTableAddDropPartitions` where you use the singular transform names.
> Also, should I change the prefix of this PR to Spark: ?
Yes please :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1659170766
@clettieri Thanks for raising this. This is a known issue, and I also run into it. However, maybe it is better to add `day` as an option to Spark since the spec is considered the source of truth.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1281119657
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
Added the tests.
I ran `/gradlew :iceberg-spark:iceberg-spark-extensions-3.4_2.12:test --tests TestAlterTablePartitionFields` locally and saw things pass. LMK if I can do anything else here (I'll try to catch up on the contributing guidelines tomorrow). Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1281095361
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
Yes, that would be perfect
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on a diff in pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1281007488
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
For the tests, what if I replicate [these](https://github.com/clettieri/iceberg/blob/docs-fix-partition-transforms/spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java#L164) but for some or all of `year, month, hour, day` ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8192: Docs: Correct partition transform functions on spec.md
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#discussion_r1280545911
##########
format/spec.md:
##########
@@ -309,10 +309,10 @@ Partition specs capture the transform from table data to partition values. This
| **`identity`** | Source value, unmodified | Any | Source type |
| **`bucket[N]`** | Hash of value, mod `N` (see below) | `int`, `long`, `decimal`, `date`, `time`, `timestamp`, `timestamptz`, `string`, `uuid`, `fixed`, `binary` | `int` |
| **`truncate[W]`** | Value truncated to width `W` (see below) | `int`, `long`, `decimal`, `string` | Source type |
-| **`year`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`month`** | Extract a date or timestamp month, as months from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`day`** | Extract a date or timestamp day, as days from 1970-01-01 | `date`, `timestamp`, `timestamptz` | `int` |
-| **`hour`** | Extract a timestamp hour, as hours from 1970-01-01 00:00:00 | `timestamp`, `timestamptz` | `int` |
+| **`years`** | Extract a date or timestamp year, as years from 1970 | `date`, `timestamp`, `timestamptz` | `int` |
Review Comment:
We need to maintain backward compatibility. I would suggest adding the singular options and also keeping the plural ones as well. They don't conflict and avoids people to run into errors. Also curious about what others think of 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1728516412
@clettieri, what about `TestCreateTable`? Unlike what is covered by new tests, we should also check the new syntax is supported by Spark without extensions.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] clettieri commented on pull request #8192: Spark: Correct partition transform functions to match spec.md
Posted by "clettieri (via GitHub)" <gi...@apache.org>.
clettieri commented on PR #8192:
URL: https://github.com/apache/iceberg/pull/8192#issuecomment-1732276358
> Looks like there are some spotless issues that fail the build.
Yep, I see that. I'll try to get to that on Monday. I'm a bit new to the JVM/Spark world and was having some trouble switching Spark environments to run Gradle locally. :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org