PARQUET-2485: Be more consistent with BYTE_ARRAY types #251

etseidl · 2024-05-28T22:07:30Z

Changes instances of 'binary' to BYTE_ARRAY where appropriate. Also fixes some uses of FIXED_LEN_BYTE_ARRAY.

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-2485
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

Changes instances of 'binary' to BYTE_ARRAY where appropriate. Also fixes some uses of FIXED_LEN_BYTE_ARRAY.

etseidl · 2024-05-28T22:10:11Z

Note: I've left 'binary' in the schema examples for now since I'm not sure if the current parquet-cli still uses 'binary' when printing file schemas.

wgtmac · 2024-05-29T01:27:29Z

Note: I've left 'binary' in the schema examples for now since I'm not sure if the current parquet-cli still uses 'binary' when printing file schemas.

Yes, I think we need to fix this as well.

etseidl · 2024-05-29T01:36:25Z

I just noticed I left in the converted type UTF8 rather than using the proper logical type name STRING. I'll fix that up tomorrow.

etseidl · 2024-05-29T16:02:16Z

Note: I've left 'binary' in the schema examples for now since I'm not sure if the current parquet-cli still uses 'binary' when printing file schemas.

Yes, I think we need to fix this as well.

I verified that parquet-cli 1.14.0 still uses 'binary' for BYTE_ARRAY. I welcome suggestions for the schema examples in LogicalTypes.md.

wgtmac · 2024-06-12T01:35:49Z

LogicalTypes.md

@@ -59,7 +60,7 @@ Compatibility considerations are mentioned for each annotation in the correspond

 ### STRING

-`STRING` may only be used to annotate the binary primitive type and indicates
+`STRING` may only be used to annotate the BYTE_ARRAY primitive type and indicates


It seems the spec is unclear about whether or not STRING and ENUM can annotate FIXED_LENGTH_BYTE_ARRAY. Literally it is reasonable to annotate FIXED_LENGTH_BYTE_ARRAY, right? I'm not sure if there is any use case in the wild.

I don't know. For DECIMAL the spec calls out "byte arrays, binary and fixed" as valid physical types. I'd take the lack of mention of fixed-length here to indicated that only the BYTE_ARRAY physical type is allowed. Do any current implementations allow fixed-length strings?

I think the existing wording is pretty clear of we take "binary primitive type" to mean BYTE_ARRAY and thus in my opinion this is not a change to the spec.

Perhaps we could send a note to [email protected] just highlighting this clarification in case someone wants to chime in and say they read it to mean FIXED_LENGTH_BYTE_ARRAY was also supported

The sense I'm getting from the M/L is that people are open to adding it, but haven't seen any evidence that any one actually supports adding STRING to FLBA. IMO we should ship this as is, and start a new thread to gauge support for actually modifying the spec to allow for fixed length strings. Given the variable width of UTF-8 characters, I'd think padding would have to be added to account for up to 4 bytes per character.

I agree with this assesment

I agree that we'd better keep it as is for now. Let me merge this. We can always change the content when there is a consensus.

wgtmac · 2024-06-12T01:37:34Z

src/main/thrift/parquet.thrift

@@ -151,14 +151,14 @@ enum ConvertedType {
  /**
   * An embedded JSON document
   *
-   * A JSON document embedded within a single UTF8 column.
+   * A JSON document embedded within a single BYTE_ARRAY(STRING) column.


Should we keep it as is? This is the deprecated ConvertedType section where UTF8 is used for string type.

Oh, good point. I'll revert.

wgtmac · 2024-06-14T02:06:25Z

LGTM cc @pitrou @alamb @tustvold

alamb

Thank you @etseidl and @wgtmac for the ping

I think this looks like a significant improvement to me -- thank you for the clarification

I left some other suggestions, but I also think this PR could be merged as as and we could make other changes a follow on PRs

LogicalTypes.md

alamb · 2024-06-17T18:06:31Z

LogicalTypes.md

 * `fixed_len_byte_array`: precision is limited by the array size. Length `n`
  can store &lt;= `floor(log_10(2^(8*n - 1) - 1))` base-10 digits
-* `binary`: `precision` is not limited, but is required. The minimum number of
+* `byte_array`: `precision` is not limited, but is required. The minimum number of


Perhaps we can change this to name the types using the same names as elswhere:

Suggested change

* `fixed_len_byte_array`: precision is limited by the array size. Length `n`

can store <= `floor(log_10(2^(8*n - 1) - 1))` base-10 digits

* `binary`: `precision` is not limited, but is required. The minimum number of

* `byte_array`: `precision` is not limited, but is required. The minimum number of

* `FIXED_LEN_BYTE_ARRAY `: precision is limited by the array size. Length `n`

can store <= `floor(log_10(2^(8*n - 1) - 1))` base-10 digits

* `BYTE_ARRAY `: `precision` is not limited, but is required. The minimum number of

Yes, the formatting here is odd...all of the type names are lower case. Also, the use of back ticks for type names is inconsistent. Perhaps (as you suggest) for this PR we can solely worry about binary->byte_array, and then do a second pass to fix capitalization and quoting.

Looking at this more, it seems I'm introducing more inconsistency with the quoting. I'll try to clean that up.

LogicalTypes.md

alamb · 2024-06-17T18:09:10Z

LogicalTypes.md

@@ -59,7 +60,7 @@ Compatibility considerations are mentioned for each annotation in the correspond

 ### STRING

-`STRING` may only be used to annotate the binary primitive type and indicates
+`STRING` may only be used to annotate the BYTE_ARRAY primitive type and indicates


I think the existing wording is pretty clear of we take "binary primitive type" to mean BYTE_ARRAY and thus in my opinion this is not a change to the spec.

Perhaps we could send a note to [email protected] just highlighting this clarification in case someone wants to chime in and say they read it to mean FIXED_LENGTH_BYTE_ARRAY was also supported

Co-authored-by: Andrew Lamb <[email protected]>

PARQUET-2485: be more consistent with BYTE_ARRAY types

969d02c

Changes instances of 'binary' to BYTE_ARRAY where appropriate. Also fixes some uses of FIXED_LEN_BYTE_ARRAY.

wgtmac approved these changes May 29, 2024

View reviewed changes

replace UTF8 converted type with STRING logical type annotation

b6ccbd5

etseidl mentioned this pull request Jun 4, 2024

PARQUET-2474: Add FIXED_SIZE_LIST logical type #241

Open

rok mentioned this pull request Jun 5, 2024

PARQUET-2492: Add binary protocol extensions #254

Merged

3 tasks

etseidl force-pushed the binary_is_byte_array branch from 927a77a to 48ff938 Compare June 10, 2024 18:51

Merge remote-tracking branch 'origin/master' into binary_is_byte_array

d0ee828

etseidl force-pushed the binary_is_byte_array branch from 48ff938 to d0ee828 Compare June 10, 2024 18:59

use STRING annotation rather than UTF8

55a7d5c

wgtmac reviewed Jun 12, 2024

View reviewed changes

etseidl added 2 commits June 11, 2024 21:26

revert JSON comment

6df2b55

Merge remote-tracking branch 'origin/master' into binary_is_byte_array

488ce4d

alamb approved these changes Jun 17, 2024

View reviewed changes

etseidl and others added 2 commits June 17, 2024 11:30

Apply suggestions from code review

6113446

Co-authored-by: Andrew Lamb <[email protected]>

be more consistent with quoting of type names

d0e8e00

alamb approved these changes Jun 17, 2024

View reviewed changes

wgtmac approved these changes Jun 18, 2024

View reviewed changes

wgtmac merged commit 18df2d4 into apache:master Jun 19, 2024
3 checks passed

etseidl deleted the binary_is_byte_array branch June 19, 2024 16:10

asfimport mentioned this pull request Jun 23, 2024

Refer to byte array types consistently #433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-2485: Be more consistent with BYTE_ARRAY types #251

PARQUET-2485: Be more consistent with BYTE_ARRAY types #251

etseidl commented May 28, 2024

etseidl commented May 28, 2024

wgtmac commented May 29, 2024

etseidl commented May 29, 2024

etseidl commented May 29, 2024

wgtmac Jun 12, 2024

etseidl Jun 12, 2024

alamb Jun 17, 2024

etseidl Jun 18, 2024

alamb Jun 18, 2024

wgtmac Jun 19, 2024

wgtmac Jun 12, 2024

etseidl Jun 12, 2024

wgtmac commented Jun 14, 2024

alamb left a comment

alamb Jun 17, 2024

etseidl Jun 17, 2024

etseidl Jun 17, 2024

alamb Jun 17, 2024

PARQUET-2485: Be more consistent with BYTE_ARRAY types #251

PARQUET-2485: Be more consistent with BYTE_ARRAY types #251

Conversation

etseidl commented May 28, 2024

Jira

Commits

Documentation

etseidl commented May 28, 2024

wgtmac commented May 29, 2024

etseidl commented May 29, 2024

etseidl commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac commented Jun 14, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment