You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "ostronaut (via GitHub)" <gi...@apache.org> on 2025/09/12 08:42:18 UTC
[PR] Fix typos in VariantShredding [parquet-format]
ostronaut opened a new pull request, #523:
URL: https://github.com/apache/parquet-format/pull/523
<!--
Thanks for opening a pull request!
If you're new to Parquet-Format, information on how to contribute can be found here: https://parquet.apache.org/docs/contribution-guidelines/contributing
Please open a GitHub issue for this pull request: https://github.com/apache/parquet-format/issues/new/choose
and format pull request title as below:
GH-${GITHUB_ISSUE_ID}: ${SUMMARY}
or simply use the title below if it is a minor issue:
MINOR: ${SUMMARY}
-->
### Rationale for this change
Fix small typos in VariantShredding documentation
### What changes are included in this PR?
Changes to MD file
### Do these changes have PoC implementations?
No
<!-- Please uncomment the line below and replace ${GITHUB_ISSUE_ID} with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Fix typos in VariantShredding [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2388671128
##########
VariantShredding.md:
##########
@@ -38,7 +38,7 @@ All field names of a Variant, whether shredded or not, must be present in the me
## Value Shredding
-Variant values are stored in Parquet fields named `value`.
+Variant values that are stored in Parquet fields named `value`.
Review Comment:
okay, will update to the original statement
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2413799425
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
I found this easier to review using the rendered preview at https://github.com/ostronaut/parquet-format/blob/patch-1/VariantShredding.md
It took me a while to figure out why we had example objects for invalid cases, but after reviewing the table some more it makes sense to me
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on PR #523:
URL: https://github.com/apache/parquet-format/pull/523#issuecomment-3423645173
Yes, apologies for the delay.
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2431490270
##########
VariantShredding.md:
##########
@@ -202,21 +202,22 @@ As a result, reads when both `value` and `typed_value` are defined may be incons
The table below shows how the series of objects in the first column would be stored:
-| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
-|------------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------|
-| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
-| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
-| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
-| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
-| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
-| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
-| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
-| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
-| null | `00` (null) | null | | | | | Object/value is null |
-| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
+|-----------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------------------------------------------------------------------|
+| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
+| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
+| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
+| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
+| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
+| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
+| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
+| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
+| null | `00` (null) | null | | | | | Object/value is null |
+| missing | null | null | | | | | Object/value is missing |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` |
+| INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID CASE: `typed_value` is null for object |
Review Comment:
I added it for more clarity, but agree, it should be understandable just with INVALID as well
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2429082780
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
Review Comment:
Added clarification to the Notes
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2430239208
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
Review Comment:
> For the first point, can we say that that the following is correct:
> [`value` of a perfectly shredded object is a variant object with no fields instead of NULL]
That's exactly my question... it's sub-optimal, but well-defined, and and I'm not immediately seeing why it would confuse or break readers?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2417484818
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
+|----------|---------------|----------------------------------------------------------------------------------------|
+| null | null | The value is missing; only valid for shredded object fields |
+| non-null | null | The value is present and may be any type (except the shredded type), including null |
Review Comment:
> Going a step further -- will (should?) the shredding spec take a stance on whether writers are required to perform widening and/or lossless narrowing conversions and/or lossy narrowing conversions of values whose types are in the same equivalence class as the typed_value column?
I thought this was discussed this and the consensus was this type or normalization was an engine concern and did not belong in writers. Writers should faithfully, write the bits given to them.
I need to re-read the spec but I agree with @scovich we should probably revert this change and bring it up for larger discussion (unless this is already stated someplace else 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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield merged PR #523:
URL: https://github.com/apache/parquet-format/pull/523
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2429050606
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
I added one more invalid case to highlight this point!
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2429002300
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
+|----------|---------------|----------------------------------------------------------------------------------------|
+| null | null | The value is missing; only valid for shredded object fields |
+| non-null | null | The value is present and may be any type (except the shredded type), including null |
Review Comment:
Thank you for your points @scovich and @emkornfield! I agree that proposed change makes the spec more strict and as this is not final decision for this matter, it would makes sense to leave it open for now! i will revert my changes for this line, thank you!
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2429143391
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
Review Comment:
These are good points! I could add them but first need to have confirmation on expected behaviour from other collaborators.
For the first point, can we say that that the following is correct:
```
| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
|-----------------------------------------|--------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|-------------------------------------------------------------------------------------------|
| INVALID CASE: `{"event_type": "login"}` | `02 00` (object with 0 fields) | non-null | null | `login` | null | null | INVALID CASE: an empty object (object with 0 fields) is present for fully shredded object |
```
cc: @alamb @emkornfield
For the second point, i think it is described in the case **one** (fully shredded object), where `value` is 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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on PR #523:
URL: https://github.com/apache/parquet-format/pull/523#issuecomment-3409990123
hey @alamb, @scovich. Can we merge the PR now?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on PR #523:
URL: https://github.com/apache/parquet-format/pull/523#issuecomment-3423496408
@emkornfield do you think we can merge this PR now?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2419304213
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
Okay, makes sense. Le me think of a suitable format for it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2414031848
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
+|----------|---------------|----------------------------------------------------------------------------------------|
+| null | null | The value is missing; only valid for shredded object fields |
+| non-null | null | The value is present and may be any type (except the shredded type), including null |
Review Comment:
This is a material change in the spec -- today a NULL `typed_value` column entry puts _no constraints whatsoever_ on the corresponding `value` column entry (for primitive types, at least).
Don't get me wrong, it's very reasonable to require this, and adding the requirement would make primitive shredding match the similar rule for partial object shredding (a field in typed_value column must never be present in objects stored in the value column).
That said, I don't know if it makes much of a difference for readers consuming numeric types, because somebody who sees an int32 `typed_value` column probably still has to read the `value` column to pick up int8 values and sufficiently narrow int64 values (and possibly even decimal values whose fractional part is zero). And once they're doing that, it doesn't actually change the algorithm at all if the `value` column can contain values that "should" have shredded and did not. Depending on the reader's casting semantics, they might need to pick up decimal and float columns as well. Maybe even string columns (i.e. spark's casting semantics would attempt to parse strings into integers).
Going a step further -- will (should?) the shredding spec take a stance on whether writers are required to perform widening and/or lossless narrowing conversions and/or lossy narrowing conversions of values whose types are in the same [equivalence class](https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#encoding-types) as the `typed_value` column?
* the variant spec places the scale 9 decimal4 value `0.035000000`, the int8 value `-5` and the int64 value `8589934592` all in the same "exact numeric" variant equivalence class, which indicates "logical equivalence" for which "user expressions... should behave the same."
* i.e. does the presence of an int32 `typed_value` column forbid the existence of int8 entries in the `value` column? Does it forbid "narrow" int64 entries such as `10` that convert losslessly to int32? What about decimal4(scale=5) values such as `15.0000` that allows a "lossless" (**) conversion to int32 but requires an expensive integer `%` operation to detect? What about decimal4(scale=5) values such as `1.23000` that could be rounded to int32?
(**) I use scare quotes for "lossless" conversion of 1.00000 to 1 because some would argue that the presence of those five trailing decimal zeros is information that would be lost by conversion to int (the input had six significant figures and the output has only one).
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
Review Comment:
What about this case?
```
| `{"event_type": "login"}` | `02 00` (object with 0 fields) | non-null | null | `login`
```
Is it legal for an object that shredded perfectly to still have an empty object in the `value` column? It's sub-optimal but is it incorrect?
Conversely, what about this case?
```
| `{"event_type": "login"}` | null | non-null | null | `login`
```
Is it allowed for an object that shredded perfectly to have NULL in the `value` column? Or is it required to have an empty object there? (guessing it's allowed to have NULL, and maybe even required to do so)
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
Review Comment:
This took a minute to decipher... it's illegal because NULL/NULL for typed_value.event_type.* columns means the `event_type` field is missing (but the `value` column value `"a"` claims this isn't an object in the first place).
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
This one is invalid because the `event_type` field is present in both `value` and `typed_value` columns of a partially shredded object.
We're actually missing a second (similar) invalid row:
```
| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | null | |
```
(because `typed_value.login` exists, an object stored in `value` must never contain that field name)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2417560990
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
My take-away from these comments is if you wouldn't mind adding english description also on why it is invalid would help readers gain context on why they are invalid rather then having to re-derive from 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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Fix typos in VariantShredding [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2388669676
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
Review Comment:
correct. plus clarification on line 71
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on PR #523:
URL: https://github.com/apache/parquet-format/pull/523#issuecomment-3411702147
LGTM, seems like others are happy with it, will merge once CI passes. Thank you @ostronaut
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2431481651
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
Review Comment:
True! Lets here from the other collaborators maybe? I can add this case to the table once we agree on it.
cc: @alamb @emkornfield
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2430310159
##########
VariantShredding.md:
##########
@@ -202,21 +202,22 @@ As a result, reads when both `value` and `typed_value` are defined may be incons
The table below shows how the series of objects in the first column would be stored:
-| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
-|------------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------|
-| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
-| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
-| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
-| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
-| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
-| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
-| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
-| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
-| null | `00` (null) | null | | | | | Object/value is null |
-| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
+|-----------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------------------------------------------------------------------|
+| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
+| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
+| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
+| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
+| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
+| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
+| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
+| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
+| null | `00` (null) | null | | | | | Object/value is null |
+| missing | null | null | | | | | Object/value is missing |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` |
+| INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object |
+| INVALID CASE: `{}` | `02 00` (object with 0 fields) | null | | | | | INVALID CASE: `typed_value` is null for object |
Review Comment:
The table is already super wide; I'm not sure ` CASE` adds enough information to be worth the space it consumes?
##########
VariantShredding.md:
##########
@@ -202,21 +202,22 @@ As a result, reads when both `value` and `typed_value` are defined may be incons
The table below shows how the series of objects in the first column would be stored:
-| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
-|------------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------|
-| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
-| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
-| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
-| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
-| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
-| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
-| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
-| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
-| null | `00` (null) | null | | | | | Object/value is null |
-| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
+|-----------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------------------------------------------------------------------|
+| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
+| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
+| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
+| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
+| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
+| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
+| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
+| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
+| null | `00` (null) | null | | | | | Object/value is null |
+| missing | null | null | | | | | Object/value is missing |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` |
+| INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object |
Review Comment:
Not understanding the "while being null" part? What about just:
> INVALID CASE: `typed_value` is present and `value` is not an object
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Fix typos in VariantShredding [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2377686001
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
Review Comment:
a bunch of these changes look like they are just adding white space/fillter?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2420307237
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
+|----------|---------------|----------------------------------------------------------------------------------------|
+| null | null | The value is missing; only valid for shredded object fields |
+| non-null | null | The value is present and may be any type (except the shredded type), including null |
Review Comment:
> > Going a step further -- will (should?) the shredding spec take a stance on whether writers are required to perform widening and/or lossless narrowing conversions and/or lossy narrowing conversions of values whose types are in the same equivalence class as the typed_value column?
>
> I thought this was discussed this and the consensus was this type or normalization was an engine concern and did not belong in writers. Writers should faithfully, write the bits given to them.
>
> I need to re-read the spec but I agree with @scovich we should probably revert this change and bring it up for larger discussion (unless this is already stated someplace else in the spec).
I remember the same discussion and the same outcome, but the spec doesn't actually say it, and the binary variant spec still has this ambiguous "equivalence class" concept that seems both very under-defined and very engine-centric (casting and equivalence semantics).
In any case, if normalization decisions are up to the engine performing the writes, then the shredding spec shouldn't constrain the engine's choices of typing. The exceptions would be to forbid ambiguity and conflicts, which it generally does a decent job of already.
So the immediate question would be: Is it ambiguous or conflicting for the `value` column to contain an Int32 value, and for `typed_value` to be NULL at that row, when `typed_value` is INT32? I think that's where we diverge from the seemingly-similar situation with partial shredding. There's no conflict -- only one value is available for that row -- and there's no ambiguity in what the value 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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #523:
URL: https://github.com/apache/parquet-format/pull/523#issuecomment-3429352247
Thank you @ostronaut @emkornfield and @scovich
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Fix typos in VariantShredding [parquet-format]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2377681966
##########
VariantShredding.md:
##########
@@ -38,7 +38,7 @@ All field names of a Variant, whether shredded or not, must be present in the me
## Value Shredding
-Variant values are stored in Parquet fields named `value`.
+Variant values that are stored in Parquet fields named `value`.
Review Comment:
I don't think this reads better then existing language.
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2414031848
##########
VariantShredding.md:
##########
@@ -65,12 +65,12 @@ The series of measurements `34, null, "n/a", 100` would be stored as:
Both `value` and `typed_value` are optional fields used together to encode a single value.
Values in the two fields must be interpreted according to the following table:
-| `value` | `typed_value` | Meaning |
-|----------|---------------|-------------------------------------------------------------|
-| null | null | The value is missing; only valid for shredded object fields |
-| non-null | null | The value is present and may be any type, including null |
-| null | non-null | The value is present and is the shredded type |
-| non-null | non-null | The value is present and is a partially shredded object |
+| `value` | `typed_value` | Meaning |
+|----------|---------------|----------------------------------------------------------------------------------------|
+| null | null | The value is missing; only valid for shredded object fields |
+| non-null | null | The value is present and may be any type (except the shredded type), including null |
Review Comment:
This is a material change in the spec -- today a NULL `typed_value` column entry puts _no constraints whatsoever_ on the corresponding `value` column entry (for primitive types, at least).
Don't get me wrong, it's very reasonable to require this, and adding the requirement would make primitive shredding match the similar rule for partial object shredding (a field in typed_value column must never be present in objects stored in the value column).
That said, I don't know if it makes much of a difference for readers consuming numeric types, because somebody who sees an int32 `typed_value` column probably still has to read the `value` column to pick up int8 values and sufficiently narrow int64 values (and possibly even decimal values whose fractional part is zero). And once they're doing that, it doesn't actually change the algorithm at all if the `value` column contains values that "should" have shredded and did not. Depending on the reader's casting semantics, they might need to pick up decimal and float columns as well. Maybe even string columns (i.e. spark's casting semantics would attempt to parse strings into integers).
Going a step further -- will (should?) the shredding spec take a stance on whether writers are required to perform widening and/or lossless narrowing conversions and/or lossy narrowing conversions of values whose types are in the same [equivalence class](https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#encoding-types) as the `typed_value` column?
* the variant spec places the scale 9 decimal4 value `0.035000000`, the int8 value `-5` and the int64 value `8589934592` all in the same "exact numeric" variant equivalence class, which indicates "logical equivalence" for which "user expressions... should behave the same."
* i.e. does the presence of an int32 `typed_value` column forbid the existence of int8 entries in the `value` column? Does it forbid "narrow" int64 entries such as `10` that convert losslessly to int32? What about decimal4(scale=5) values such as `15.0000` that allows a "lossless" (**) conversion to int32 but requires an expensive integer `%` operation to detect? What about decimal4(scale=5) values such as `1.23000` that could be rounded to int32?
(**) I use scare quotes for "lossless" conversion of 1.00000 to 1 because some would argue that the presence of those five trailing decimal zeros is information that would be lost by conversion to int (the input had six significant figures and the output has only one).
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2431502474
##########
VariantShredding.md:
##########
@@ -202,21 +202,22 @@ As a result, reads when both `value` and `typed_value` are defined may be incons
The table below shows how the series of objects in the first column would be stored:
-| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
-|------------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------|
-| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
-| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
-| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
-| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
-| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
-| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
-| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
-| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
-| null | `00` (null) | null | | | | | Object/value is null |
-| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| Event object | `value` | `typed_value` | `typed_value.event_type.value` | `typed_value.event_type.typed_value` | `typed_value.event_ts.value` | `typed_value.event_ts.typed_value` | Notes |
+|-----------------------------------------------------------------------------------|-----------------------------------|---------------|--------------------------------|--------------------------------------|------------------------------|------------------------------------|--------------------------------------------------------------------------------------------------------------|
+| `{"event_type": "noop", "event_ts": 1729794114937}` | null | non-null | null | `noop` | null | 1729794114937 | Fully shredded object |
+| `{"event_type": "login", "event_ts": 1729794146402, "email": "user@example.com"}` | `{"email": "user@example.com"}` | non-null | null | `login` | null | 1729794146402 | Partially shredded object |
+| `{"error_msg": "malformed: ..."}` | `{"error_msg", "malformed: ..."}` | non-null | null | null | null | null | Object with all shredded fields missing |
+| `"malformed: not an object"` | `malformed: not an object` | null | | | | | Not an object (stored as Variant string) |
+| `{"event_ts": 1729794240241, "click": "_button"}` | `{"click": "_button"}` | non-null | null | null | null | 1729794240241 | Field `event_type` is missing |
+| `{"event_type": null, "event_ts": 1729794954163}` | null | non-null | `00` (field exists, is null) | null | null | 1729794954163 | Field `event_type` is present and is null |
+| `{"event_type": "noop", "event_ts": "2024-10-24"}` | null | non-null | null | `noop` | `"2024-10-24"` | null | Field `event_ts` is present but not a timestamp |
+| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
+| null | `00` (null) | null | | | | | Object/value is null |
+| missing | null | null | | | | | Object/value is missing |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID CASE: Shredded field is present in `value` |
+| INVALID CASE: `{"event_type": "login"}` | `{"event_type": "login"}` | null | | | | | INVALID CASE: Shredded field is present in `value`, while `typed_value` is null |
+| INVALID CASE: `"a"` | `"a"` | non-null | null | null | null | null | INVALID CASE: `typed_value` is present for non-object while being null, even though `value` is not an object |
Review Comment:
Rephrased as per suggestion!
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Improve wording about error cases in `VariantShredding` [parquet-format]
Posted by "ostronaut (via GitHub)" <gi...@apache.org>.
ostronaut commented on code in PR #523:
URL: https://github.com/apache/parquet-format/pull/523#discussion_r2429054451
##########
VariantShredding.md:
##########
@@ -214,9 +214,9 @@ The table below shows how the series of objects in the first column would be sto
| `{ }` | null | non-null | null | null | null | null | Object is present but empty |
| null | `00` (null) | null | | | | | Object/value is null |
| missing | null | null | | | | | Object/value is missing |
-| INVALID | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
-| INVALID | `"a"` | non-null | null | null | null | null | INVALID: `typed_value` is present for non-object |
-| INVALID | `02 00` (object with 0 fields) | null | | | | | INVALID: `typed_value` is null for object |
+| INVALID CASE: `{"event_type": "login", "event_ts": 1729795057774}` | `{"event_type": "login"}` | non-null | null | `login` | null | 1729795057774 | INVALID: Shredded field is present in `value` |
Review Comment:
Notes now flighting all invalid cases
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org