iemejia commented on code in PR #574:
URL: https://github.com/apache/parquet-format/pull/574#discussion_r3359148602
##########
VariantEncoding.md:
##########
@@ -407,20 +414,20 @@ The Decimal type contains a scale, but no precision. The
implied precision of a
| NullType | null | `0` | UNKNOWN
| none
|
| Boolean | boolean (True) | `1` | BOOLEAN
| none
|
| Boolean | boolean (False) | `2` | BOOLEAN
| none
|
-| Exact Numeric | int8 | `3` | INT(8,
signed) | 1 byte
|
-| Exact Numeric | int16 | `4` | INT(16,
signed) | 2 byte little-endian
|
-| Exact Numeric | int32 | `5` | INT(32,
signed) | 4 byte little-endian
|
-| Exact Numeric | int64 | `6` | INT(64,
signed) | 8 byte little-endian
|
+| Exact Numeric | int8 | `3` | INT(8, true)
| 1-byte
|
+| Exact Numeric | int16 | `4` | INT(16, true)
| 2-byte little-endian
|
+| Exact Numeric | int32 | `5` | INT(32, true)
| 4-byte little-endian
|
+| Exact Numeric | int64 | `6` | INT(64, true)
| 8-byte little-endian
|
| Double | double | `7` | DOUBLE
| IEEE little-endian
|
-| Exact Numeric | decimal4 | `8` |
DECIMAL(precision, scale) | 1 byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
-| Exact Numeric | decimal8 | `9` |
DECIMAL(precision, scale) | 1 byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
-| Exact Numeric | decimal16 | `10` |
DECIMAL(precision, scale) | 1 byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
-| Date | date | `11` | DATE
| 4 byte little-endian
|
+| Exact Numeric | decimal4 | `8` |
DECIMAL(precision, scale) | 1-byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
+| Exact Numeric | decimal8 | `9` |
DECIMAL(precision, scale) | 1-byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
+| Exact Numeric | decimal16 | `10` |
DECIMAL(precision, scale) | 1-byte scale in range [0, 38], followed by
little-endian unscaled value (see decimal table) |
+| Date | date | `11` | DATE
| 4-byte little-endian
|
| Timestamp | timestamp | `12` |
TIMESTAMP(isAdjustedToUTC=true, MICROS) | 8-byte little-endian
|
| TimestampNTZ | timestamp without time zone | `13` |
TIMESTAMP(isAdjustedToUTC=false, MICROS) | 8-byte little-endian
|
| Float | float | `14` | FLOAT
| IEEE little-endian
|
-| Binary | binary | `15` | BINARY
| 4 byte little-endian size, followed by bytes
|
-| String | string | `16` | STRING
| 4 byte little-endian size, followed by UTF-8 encoded bytes
|
+| Binary | binary | `15` | BYTE_ARRAY
| 4-byte little-endian size, followed by bytes
|
Review Comment:
Thanks for verifying. Yes, the Parquet thrift definition
(`parquet.thrift:39`) uses `BYTE_ARRAY` as the physical type name, and the
official documentation at parquet.apache.org confirms this. The previous text
used `BINARY` which is actually the name of the deprecated `ConvertedType` enum
value (used for the old logical type annotation), not the physical type. Since
this column in the table is labeled "Equivalent Parquet Type" and refers to the
physical type, `BYTE_ARRAY` is the correct name.
##########
VariantShredding.md:
##########
@@ -85,8 +85,8 @@ Shredded values must use the following Parquet types:
| Variant Type | Parquet Physical Type | Parquet
Logical Type |
|-----------------------------|-----------------------------------|--------------------------|
| boolean | BOOLEAN |
|
-| int8 | INT32 | INT(8,
signed=true) |
-| int16 | INT32 | INT(16,
signed=true) |
+| int8 | INT32 | INT(8,
true) |
Review Comment:
I changed it to match the notation used in `LogicalTypes.md` (line 100: "a
signed integer with bit width of 8 is defined as `INT(8, true)`"). The
`INT(bitWidth, isSigned)` notation is what the spec uses consistently. I agree
`signed=true` is arguably more readable in isolation, but consistency with the
canonical spec notation seems more important for a formal specification doc.
Happy to hear other opinions.
##########
VariantShredding.md:
##########
@@ -294,7 +294,7 @@ def construct_variant(metadata: Metadata, value: Variant,
typed_value: Any) -> V
# this is a shredded object
object_fields = {
name: construct_variant(metadata, field.value,
field.typed_value)
- for (name, field) in typed_value
+ for (name, field) in typed_value.items()
Review Comment:
In Python, iterating over a `dict` directly (`for x in my_dict`) yields only
the keys, not `(key, value)` tuples. The code uses `typed_value` as a dict
(line 293: `isinstance(typed_value, dict)`) and then unpacks both `name` and
`field` from each iteration — this only works with `.items()`. The original
code would raise a `ValueError: not enough values to unpack` at runtime.
Additionally, `typed_value.keys()` is called on line 303 which confirms it's
expected to be a dict. So yes, this is a bug fix — the original code wouldn't
work as written.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
