You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2025/03/25 18:58:27 UTC
[PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Fokko opened a new pull request, #12644:
URL: https://github.com/apache/iceberg/pull/12644
Changes around the multi-argument transforms, mainly two things:
- **Up for debate**. The spec does not point out an actual implementations of transforms that accept multiple arguments. From the existing transforms, the only contender is the bucket transform. Should we include this in the V3 spec? It will only allow to prune metadata if you do an equality expression on all the fields that are part of the transform.
- Along the way, we've [removed something that we did not intend](https://github.com/apache/iceberg/blame/10ec85ffcab6ac12ecac7fc6df9c3e4139f857b7/format/spec.md#L1139-L1140). First we allowed to write source-id and source-ids based on the number of arguments. This has been changed to only allow source-ids for V3 in a PR that introduces backward compatibility. I think this makes the JSON parsers/producers more complex than needed (specifically PyIceberg). Also, in Java we would need to plumb down the table version to the [PartitionSpecParser.java](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/PartitionSpecParser.java#L101). I think it would be great to simplify this.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047180457
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | optional | **`transform`** | `JSON string` | `bucket[4]` |
Review Comment:
Transform is still required, right? This looks like a typo and you meant to change `source-ids` to optional?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2027416683
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
This sounds reasonable to me, except that I think `source-ids` should be required all the time.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037902209
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
While starting the update of core, I think `source-ids` should be required.
The workflow would be:
1. For the reader:
1.1. We read `source-ids`
1.2. If `source-ids` is not present or null, we read `source-id` (for backward compatibility)
2. For the writer:
2.1. We write only and always `source-ids` (`source-id` doesn't exist anymore on new writer)
NB: `source-ids` is a String field as it contains a list, e.g. `1,3,5` for instqnce (`source-id` is a int field as it's a single value field).
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047344672
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | optional | **`transform`** | `JSON string` | `bucket[4]` |
Review Comment:
Yes, I was one-off, 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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2045308728
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
I thought from the other discussion we are now changing this to source-id is required for single arg transforms and source-ids is required for multi arg. So both are optional but are still "must write" language in 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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2014455784
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
These are implementation details but I would suggest if source-id and source-ids are both there
source-ids = list(source-id) - Fine on read, on write omit source-id
source-ids != list(source-id) - Error on read
source-id = x && source-ids == null - Fine on read, promote to list(source-id) on write
source-id = null && source-ids = list - Spec compliant config
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2042855103
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
Iceberg only supports forward compatibility, so V3 readers should be able to read V2, but not the other way around. Still, requiring the table version for serializing the partition-spec does not sound ideal to me. I've changed the PR back to depend on the serialization based on the number of arguments, rather than the table version.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037902209
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
While starting the update of core, I think `source-ids` should be required, and `source-id` is required only for the single arg transforms.
The workflow would be:
1. For the reader:
1.1. We read `source-ids`
1.2. If `source-ids` is not present or null, we read `source-id` (for backward compatibility)
2. For the writer:
2.1. We write always `source-ids` (`source-id` doesn't exist anymore on new writer)
2.2. For single arg transform, we also write `source-id`
NB: `source-ids` is a JSON list of ints field, e.g. `[1,3,5]` for instance (`source-id` is a int field as it's a single value field).
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037902209
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
While starting the update of core, I think `source-ids` should be required.
The workflow would be:
1. For the reader:
1.1. We read `source-ids`
1.2. If `source-ids` is not present or null, we read `source-id` (for backward compatibility)
2. For the writer:
2.1. We write always `source-ids` (`source-id` doesn't exist anymore on new writer)
2.2. For single arg transform, we also write `source-id`
NB: `source-ids` is a JSON list of ints field, e.g. `[1,3,5]` for instance (`source-id` is a int field as it's a single value field).
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016173037
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
> That way the spec reader code doesn't have to be version dependent.
My goal was to be completely independent of the table version, also on the write side. Why not entirely remove this constraint by writing `source-id` or `source-ids` based on the number of arguments? :) I think they should be written mutually exclusive, to avoid any confusion.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016173037
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
> That way the spec reader code doesn't have to be version dependent.
My goal was to be completely independent of the table version, also on the write side. Why not entirely remove this constraint by writing `source-id` or `source-ids` based on the number of arguments? :)
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016270408
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | required¹ | **`transform`** | `JSON string` | `bucket[4]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| required | required | required | **`direction`** | `JSON string` | `asc` |
| required | required | required | **`null-order`** | `JSON string` | `nulls-last`|
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
+Notes:
+
+1. For sort fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
This was the idea:
But I guess it is not obvious? I think @wgtmac raised the same question in https://github.com/apache/iceberg/pull/12644#discussion_r2015906621?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016273706
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
I took inspiration from Apache Hive. I think it plays well with the hashing.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037902209
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
While starting the update of core, I think `source-ids` should be required.
The workflow would be:
1. For the reader:
1.1. We read `source-ids`
1.2. If `source-ids` is not present or null, we read `source-id` (for backward compatibility)
2. For the writer:
2.1. We write only and always `source-ids` (`source-id` doesn't exist anymore on new writer)
NB: `source-ids` is a JSON list of ints field, e.g. `[1,3,5]` for instance (`source-id` is a int field as it's a single value field).
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "sfc-gh-bhannel (via GitHub)" <gi...@apache.org>.
sfc-gh-bhannel commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2014937438
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
-| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
+| | | required¹ | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| | required | required | **`field-id`** | `JSON int` | 1000 |
| required | required | required | **`name`** | `JSON string` | `id_bucket` |
| required | required | required | **`transform`** | `JSON string` | `bucket[16]` |
+Notes:
+
+1. For partition fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
Would it be more consistent to always use source-ids for v3 writers? Or do you want to use source-id to allow v3 writers to be compatible with v2 readers if you don't use v3 features?
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | required¹ | **`transform`** | `JSON string` | `bucket[4]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| required | required | required | **`direction`** | `JSON string` | `asc` |
| required | required | required | **`null-order`** | `JSON string` | `nulls-last`|
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
+Notes:
+
+1. For sort fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
Did you mean to make the `transform` row refer to this footnote? I would expect it to be the `source-id` and `source-ids` rows.
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
I think it would be good to be explicit about how to handle nulls and dropped columns. Should it be calculated as if `hash(null) == 0`?
It also might be worth adding parentheses for those who don't recall the precedence order between XOR and modulo.
Also - good to meet you, and thanks for working on this! I'm a developer at Snowflake working on Iceberg functionality, and we really appreciate how detailed the spec is!
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021373556
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
`source-ids` is `format-version≥3`, otherwise you need to write `source-id`'s, otherwise you might break v1 and v2 readers that are not aware of `source-ids` yet.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037907676
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | optional | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| | required | required | **`field-id`** | `JSON int` | 1000 |
| required | required | required | **`name`** | `JSON string` | `id_bucket` |
| required | required | required | **`transform`** | `JSON string` | `bucket[16]` |
+Notes:
+
+1. For partition fields with a transform with a single argument, `source-id` can still be written.
Review Comment:
We should write both `source-id` and `source-ids` for single arg.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037909976
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
Why not only writing `source-ids`, even for single argument (single value list) ?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#issuecomment-2773084895
I wonder should we remove multi-bucket into separate pr, to allow the source-id part to get in?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2042840223
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
No, I mean that TableVersion 3 should create both `source-id` and `source-ids` (for backward compatibility).
My initial idea was to only use `source-ids` for TableVersion 3 (even for single arg transform), but it would not work with reader from previous version.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037909976
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
That's what we discussed on the mailing list. That's OK, but we should always write `source-ids`.
It means for single arg transform we write both `source-ids` and `source-id`: it's redundant but fine.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#issuecomment-2814073343
Looks good to me, thanks @Fokko !
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047342567
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | optional | **`transform`** | `JSON string` | `bucket[4]` |
+| required | required | optional | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
Review Comment:
```suggestion
| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
| required | required | optional | **`source-id`** | `JSON int` | 1 |
| | | optional | **`source-ids`** | `JSON list of ints` | `[1,2]` |
```
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#issuecomment-2756063498
Hi @Fokko i havent take a look yet at spec change, but for multi bucket we had some discussions last year. For reference the pr is here : https://github.com/apache/iceberg/pull/8259 with more discussion from: https://github.com/apache/iceberg/pull/8579
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021373556
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
`source-ids` is `format-version≥3`, for `v{1,2}` you need to write `source-id`, otherwise you might break v1 and v2 readers that are not aware of `source-ids` yet.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2018926856
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
I think we can be strict on the write side ... unless we want to keep both fields indefinitely which I'm not sure is super beneficial?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021369062
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
I'm not sure I understand why we would need the format version though? Can't we always just read both as I stated above? Then we only write source-ids. That would also be version independent
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021528707
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Ah you meant that we need the format version for the write path! Got it, I thought we were talking about the read side, let me think about that.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2045308728
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
I thought from the other discussion we are now changing this to source-id is required for single arg transforms and source-ids is required for multi arg. So both are optional but are still "must write" language in the spec
* `source-id` must be written in the case of single-argument transforms.
* `source-ids` must be written in the case of a multi-argument transform.
https://github.com/apache/iceberg/pull/12644/files#diff-36347a47c3bf67ea2ef6309ea96201814032d21bb5f162dfae4045508c15588aR1425
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047563083
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
@RussellSpitzer I agree, thanks for pointing out. I've updated the wording, 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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2015906621
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | required¹ | **`transform`** | `JSON string` | `bucket[4]` |
Review Comment:
I guess the note is in the wrong place?
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
If both `source-ids` and `source-id` are written for single-arg transform, a v3-supported reader can take a shortcut on `source-ids` and only fall back to `source-id` when `source-ids` is missing. Isn't this a desirable feature? Though someone may argue that we have to check the consistency between them like the above discussion.
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in case of multi-argument transforms.
Review Comment:
```suggestion
* `source-ids` was added and is required in the case of multi-argument transforms.
```
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016178968
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
```suggestion
5. For multi-argument bucketing, the hashes are `xor`'ed: `(hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
```
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047560528
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and should be written in case of a multi-argument transform.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
```suggestion
* `source-ids` was added and must be written in the case of a multi-argument transform.
* `source-id` must be written in the case of single-argument transforms.
```
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #12644:
URL: https://github.com/apache/iceberg/pull/12644
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2012819075
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Can't we still have a reader which reads both source-id and source-ids? Basically in our motto of reading potentially out of spec but writing in spec, we can always check if source-id is set as well as source-ids. The only time we need to break is if the two don't match.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2015866843
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
Is there a specific reason to choose `xor` but not other arithmetic operations?
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
Based on the fact that bucket(null) returns null, if any value is null then the entire hash value should be null? I agree that this should be explicit.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2042524703
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
> 2.1. We write always source-ids (source-id doesn't exist anymore on new writer)
I had a similar question below. Would this also be true for `TableVersion≤2`?
> 1. For partition fields with a transform with a single argument, only `source-id` is written. In case of a multi-argument transform, only `source-ids` is written.
Initially, there was confusion around making both fields required, and this was not incorrect. They are optional based on the number of arguments, and this is fully deterministic. It would still follow @jbonofre's reader workflow, but only produce one field based on the number of arguments.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021334130
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Ah, I see the confusion. Some examples. In the case of a single arg:
```json
{"field-id": 1000, "source-id": 1, "name": "bucket_user_id", "transform": "bucket[16]"}
```
In the case of a multi-arg:
```json
{"field-id": 1000, "source-ids": [1, 2], "name": "bucket_user_id", "transform": "bucket[16]"}
```
The benefit is that we don't have to carry the `format-version` to the serializers. So, they are mutually exclusive, and one of them is required, indicated by the `¹`:
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021334130
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Ah, I see the confusion. Some examples. In the case of a single arg:
```json
{"field-id": 1000, "source-id": 1, "name": "bucket_user_id", "transform": "bucket[16]"}
```
In the case of a multi-arg:
```json
{"field-id": 1000, "source-ids": [1, 2], "name": "bucket_user_id", "transform": "bucket[16]"}
```
So, they are mutually exclusive, and one of them is required, indicated by the `¹`. The benefit is that we don't have to carry the `format-version` to the serializers.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016178968
##########
format/spec.md:
##########
@@ -540,7 +540,7 @@ Notes:
2. The width, `W`, used to truncate decimal values is applied using the scale of the decimal column to avoid additional (and potentially conflicting) parameters.
3. Strings are truncated to a valid UTF-8 string with no more than `L` code points.
4. In contrast to strings, binary values do not have an assumed encoding and are truncated to `L` bytes.
-
+5. For multi-argument bucketing, the hashes are `xor`'ed: `hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`.
Review Comment:
Great to meet you as well @sfc-gh-bhannel, thanks for jumping in here.
> It also might be worth adding parentheses for those who don't recall the precedence order between XOR and modulo.
Thanks, there was a bracket missing actually ;)
The spec states (just below the table):
> All transforms must return `null` for a `null` input value.
I think we can amend this for the multi-arg transforms that we leave out `null` values, but we can still hash the other not-null fields.
```suggestion
5. For multi-argument bucketing, the hashes for the not-null input values are `xor`'ed: `(hash(col1) ⊕ hash(col2) ⊕ ... ⊕ hash(colN)) % W`. The transform will return `null` when all input values are `null`.
```
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016866335
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Oh, sorry for misclarification, my question is for multi arg transform like `bucket(a, b, c)`, which `source id`'s value should we write since it's now changed to required.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047182098
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
I agree with the current changes to use both `source-id` and `source-ids` depending on whether the transform is multi-arg.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2044291208
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
Yes. That was my initial proposal: we write source-ids only for multi arg transform and source-id for single arg. Reader reads source-ids first and fall back to source-id. Let's do that :)
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2044288484
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Thanks for clarifying on Slack and here. I agree with the approach.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2041801699
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
Just to clarify, do you mean also producing `source-ids` when `TableVersion≤2`?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2022289856
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Sorry for not being clearer! Give it some time to simmer and let us know :)
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2021373556
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
`source-ids` is `format-version≥3`, for `v{1,2}` you need to write `source-id`, otherwise you might break v1 and v2 readers that are not aware of `source-ids`.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2028181000
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | required¹ | **`transform`** | `JSON string` | `bucket[4]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| required | required | required | **`direction`** | `JSON string` | `asc` |
| required | required | required | **`null-order`** | `JSON string` | `nulls-last`|
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
+Notes:
+
+1. For sort fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
Thanks, and I agree that making both fields required is the main source of confusion here. Having `source-ids` required for V3 and not writing it for earlier versions requires us to know about the `format-version` when serializing the `partition-spec`/`sort-order` to json`.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2027415792
##########
format/spec.md:
##########
@@ -1453,13 +1457,15 @@ Each sort field in the fields list is stored as an object with the following pro
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|-------------|
-| required | required | required | **`transform`** | `JSON string` | `bucket[4]` |
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | required¹ | **`transform`** | `JSON string` | `bucket[4]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| required | required | required | **`direction`** | `JSON string` | `asc` |
| required | required | required | **`null-order`** | `JSON string` | `nulls-last`|
-In v3 metadata, writers must use only `source-ids` because v3 requires reader support for multi-arg transforms.
+Notes:
+
+1. For sort fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
In other cases in the spec, we use `[1]` or similar to reference notes. I'd prefer keeping it that way. I also think it would make sense to have a "notes" column and put this in the table. That's probably easier.
Also, I don't think that this should be required. If it can be omitted, then it must be optional. With the current changes, I think this would make both of them optional and state that at least one must be present. Since that's a little strange, we may want to change it so that `source-id` is optional and `source-ids` is required. In that case, `source-ids` would always have the full set of IDs and `source-id` would only be present if there is only one source field 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.
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2047368778
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
I agree, I just think this particular line makes it sound optional so I wanted to remove the word `should`
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2037907676
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
+| required | required | optional | **`source-id`** | `JSON int` | 1 |
| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| | required | required | **`field-id`** | `JSON int` | 1000 |
| required | required | required | **`name`** | `JSON string` | `id_bucket` |
| required | required | required | **`transform`** | `JSON string` | `bucket[16]` |
+Notes:
+
+1. For partition fields with a transform with a single argument, `source-id` can still be written.
Review Comment:
Are we sure there ? Regarding the other discussion, writer should only write `source-ids` even with single value list.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2027429855
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
I noted this below, but I think we need to fix the issue where we have 2 required fields that may not actually be present. I think we should make `source-ids` required and always write it for v3. I think it is best to have simple and clear rules.
Then the question is how to handle `source-id`. If we are saying that `source-ids` is required and should always be used, then it doesn't make a lot of sense to me to continue writing `source-id`, but at the same time I don't really have a problem if it is still produced, but ignored. I think I would recommend:
1. First read `source-ids`
2. If it is not present, fall back to reading `source-id`
3. Always write `source-ids` in newer versions
4. If it is hard to write only `source-ids` then it is okay to also write `source-id`
I think that would make it so we don't need to worry about the conflict between the two fields. It is also fairly permissive when reading, if we want that. I think it would also be fine to validate that `source-ids` is always used for v3 tables, but it sounds like that's additional parser work.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#issuecomment-2775432019
@szehon-ho I think that's a good idea. Let me rework the 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.
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2042855103
##########
format/spec.md:
##########
@@ -1605,13 +1611,8 @@ All readers are required to read tables with unknown partition transforms, ignor
Writing v3 metadata:
* Partition Field and Sort Field JSON:
- * `source-ids` was added and is required
- * `source-id` is no longer required and should be omitted; always use `source-ids` instead
-
-Reading v1 or v2 metadata for v3:
-
-* Partition Field and Sort Field JSON:
- * `source-ids` should default to a single-value list of the value of `source-id`
+ * `source-ids` was added and is required in the case of multi-argument transforms.
+ * `source-id` should still be written in the case of single-argument transforms.
Review Comment:
Iceberg only supports forward compatibility, so V3 readers should be able to read V2, but not the other way around. Still, requiring the table version for serializing the partition-spec does not sound ideal to me. I've changed the PR back to depend on the serialization based on the number of arguments, rather than the table version. I think that should also be the easiest implementation-wise, and we avoid discrepancies between `source-id` and `source-ids`. WDYT?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2014479209
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Yeah my suggestion is that we still say in V3 that source-id is omitted but in the implementation we can still handle an out of spec metadata if we come across one. That way the spec reader code doesn't have to be version dependent.
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2014473873
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
Yes, I agree that we should break if they doesn't match. We have a similar situation with `schema` and `schemas` with `current-schema-id` that might point to two different schemas.
When we write both, we can resolve that, but I think we don't need them both?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2014091788
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
What should the value be if `source-ids` is present?
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2016265056
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
-| | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` |
+| required | required | required¹ | **`source-id`** | `JSON int` | 1 |
+| | | required¹ | **`source-ids`** | `JSON list of ints` | `[1,2]` |
| | required | required | **`field-id`** | `JSON int` | 1000 |
| required | required | required | **`name`** | `JSON string` | `id_bucket` |
| required | required | required | **`transform`** | `JSON string` | `bucket[16]` |
+Notes:
+
+1. For partition fields with a transform with a single argument, the ID of the source field is set on `source-id`, and `source-ids` is omitted.
Review Comment:
This makes the serialization logic much easier, since we don't need to know about the table version. Otherwise, we would need to plumb down the table version into the [serializer](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/PartitionSpecParser.java#L101).
--
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
Re: [PR] Spec: Allow the use of `source-id` in V3 [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12644:
URL: https://github.com/apache/iceberg/pull/12644#discussion_r2025182220
##########
format/spec.md:
##########
@@ -1414,12 +1414,16 @@ Each partition field in `fields` is stored as a JSON object with the following p
| V1 | V2 | V3 | Field | JSON representation | Example |
|----------|----------|----------|------------------|---------------------|--------------|
-| required | required | omitted | **`source-id`** | `JSON int` | 1 |
Review Comment:
I still am not sure I like that we flip flop behavior just for the implementations sake, but it does give us some nice backwards compatibility. I think i'm in favor but It does kind of make it very easy for an engine to just avoid multi-arg transforms if they want to...
--
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