GH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64 #15244

wgtmac · 2023-01-07T15:47:00Z

As the parquet specs states, DECIMAL can be used to annotate the following types:

int32: for 1 <= precision <= 9
int64: for 1 <= precision <= 18; precision < 10 will produce a warning
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 bytes to store the unscaled value should be used.

The aim of this patch is to provide a writer option to use int32 to annotate decimal when 1 <= precision <= 9 and int64 when 10 <= precision <= 18.

Closes: [C++][Parquet] Parquet writer supports writing int32/int64 for decimal type #15239

github-actions · 2023-01-07T15:47:22Z

Closes: [C++][Parquet] Parquet writer supports writing int32/int64 for decimal type #15239

github-actions · 2023-01-07T15:47:24Z

⚠️ GitHub issue #15239 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-01-07T15:47:25Z

⚠️ GitHub issue #15239 has no components, please add labels for components.

wgtmac · 2023-01-08T11:15:01Z

@pitrou @emkornfield @wjones127 Could you please take a look? Thanks!

cc @liukun4515

wjones127 · 2023-01-09T20:10:15Z

cpp/src/parquet/properties.h

@@ -199,7 +199,8 @@ class PARQUET_EXPORT WriterProperties {
          pagesize_(kDefaultDataPageSize),
          version_(ParquetVersion::PARQUET_2_4),
          data_page_version_(ParquetDataPageVersion::V1),
-          created_by_(DEFAULT_CREATED_BY) {}
+          created_by_(DEFAULT_CREATED_BY),
+          integer_annotate_decimal_(false) {}


How widespread is support? IIUC, this allows for more compact decimal representation when available, which seems like it would be nice to have in most cases. The only reason not to, IMO is if most other implementations don't support reading this.

Another reason to keep the default false, is to not change enable code paths that weren't used before when people update there library version.

I understand the desire for stability, but would like to have a pathway for improved defaults to be released. It's unfortunate we always do a major release while still trying to respect stability, as there's no clear good time to make breaking changes like this.

I agree this is a hard problem, and don't have any guidance. The one thing I would say is that if Arrow is intended to be developer facing, then IMO developers looking to upgrade versions we should be following the "least surprise" principle as much as possible. Other opinions are valid here.

Hi,
Some belated comments:

Maybe it's because I am not a native English speaker, but I have a hard time understanding what "integer annotate decimal" means. Is "annotate" a word here?

I agree that the defaults should remain false for the time being.

While enabling this does allow for a more compact representation, compression might at least partially alleviate that (though unfortunately FIXED_LEN_BYTE_ARRAY doesn't seem to support any of the delta encodings)

Filed https://issues.apache.org/jira/browse/PARQUET-2231 because parquet-mr allows DELTA_BYTE_ARRAY on FIXED_LEN_BYTE_ARRAY, while the spec doesn't say so...

wjones127

This generally looks good, but would like to see an additional test for the null code paths.

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

…lColumn

wjones127

I found Decimal256 doesn't work with this code path if store_schema is set. Could you also address that? Here is a unit test that should reproduce:

TEST(ArrowReadWrite, Decimal256AsInt) {
  using ::arrow::Decimal256;
  using ::arrow::field;

  auto type = ::arrow::decimal256(8, 4);

  const char* json = R"(["1.0000", null, "-1.2345", "-1000.5678",
                         "-9999.9999", "9999.9999"])";
  auto array = ::arrow::ArrayFromJSON(type, json);
  auto table = ::arrow::Table::Make(::arrow::schema({field("root", type)}), {array});

  parquet::WriterProperties::Builder builder;
  // Enforce integer type to annotate decimal type
  auto writer_properties = builder.enable_integer_annotate_decimal()->build();
  auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();

  CheckConfiguredRoundtrip(table, table, writer_properties, props_store_schema);
}

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

liukun4515 · 2023-01-11T02:57:11Z

cpp/src/parquet/arrow/schema.cc

      const auto& decimal_type = static_cast<const ::arrow::DecimalType&>(*field->type());
      precision = decimal_type.precision();
      scale = decimal_type.scale();
-      length = DecimalType::DecimalSize(precision);
+      if (properties.integer_annotate_decimal() && 1 <= precision && precision <= 18) {


Does c++ arrow support converting int32/int64 to decimal128 and decimal256？

Not sure what you mean by that. This PR is how the data is encoded in the buffers, not what logical type is saved in the Parquet file. When read back to Arrow it will always come out as a Decimal, so there's no need for conversion.

Does c++ arrow support converting int32/int64 to decimal128 and decimal256？

decimal128/decimal256 support construction from integers. But the parquet itself does not use an integer array to represent decimal values.

Got it, thanks

wgtmac · 2023-01-11T05:42:05Z

I found Decimal256 doesn't work with this code path if store_schema is set. Could you also address that? Here is a unit test that should reproduce:

TEST(ArrowReadWrite, Decimal256AsInt) {
  using ::arrow::Decimal256;
  using ::arrow::field;

  auto type = ::arrow::decimal256(8, 4);

  const char* json = R"(["1.0000", null, "-1.2345", "-1000.5678",
                         "-9999.9999", "9999.9999"])";
  auto array = ::arrow::ArrayFromJSON(type, json);
  auto table = ::arrow::Table::Make(::arrow::schema({field("root", type)}), {array});

  parquet::WriterProperties::Builder builder;
  // Enforce integer type to annotate decimal type
  auto writer_properties = builder.enable_integer_annotate_decimal()->build();
  auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();

  CheckConfiguredRoundtrip(table, table, writer_properties, props_store_schema);
}

Fixed and added the test case. Please take a look again. Thanks! @wjones127

wjones127

Looks good. Thank you @wgtmac!

ursabot · 2023-01-12T10:53:34Z

Benchmark runs are scheduled for baseline = 0da51b7 and contender = 7d17a5b. 7d17a5b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.51% ⬆️0.0%] test-mac-arm
[Finished ⬇️1.02% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.59% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7d17a5b0 ec2-t3-xlarge-us-east-2
[Finished] 7d17a5b0 test-mac-arm
[Finished] 7d17a5b0 ursa-i9-9960x
[Finished] 7d17a5b0 ursa-thinkcentre-m75q
[Finished] 0da51b72 ec2-t3-xlarge-us-east-2
[Finished] 0da51b72 test-mac-arm
[Finished] 0da51b72 ursa-i9-9960x
[Finished] 0da51b72 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-01-12T10:53:54Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

pitrou · 2023-01-16T10:01:07Z

cpp/src/parquet/properties.h

@@ -450,6 +451,22 @@ class PARQUET_EXPORT WriterProperties {
      return this->disable_statistics(path->ToDotString());
    }

+    /// Enable integer type to annotate decimal type as below:


Is "Enable X to do Y" correct English? Just asking, as I'm not a native English speaker, but I would intuitively have expected "Allow X to do Y".

Also, it would be nice if the docstring was more explanatory as to the consequences (the reader may not know that this may make the column more compact, and not be readable with previous Arrow C++ versions).

mapleFU · 2023-03-14T03:02:53Z

Should we DCHECK the precision when writing arrow? Seems I meet a bug that my precision is wrong, but it write success as Decimal128, and read as int32... @wgtmac

wgtmac · 2023-03-14T03:35:11Z

Should we DCHECK the precision when writing arrow? Seems I meet a bug that my precision is wrong, but it write success as Decimal128, and read as int32... @wgtmac

Could you please provide the process to reproduce it? Or you can directly submit a PR if it is straight-forward.

mapleFU · 2023-03-14T04:31:49Z

Sure, I will submit a tiny check tonight

apacheGH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64

b4241ea

github-actions bot added Component: C++ Component: Parquet labels Jan 7, 2023

wjones127 reviewed Jan 9, 2023

View reviewed changes

wjones127 requested changes Jan 9, 2023

View reviewed changes

cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated Show resolved Hide resolved

add test TestIntegerAnnotateDecimalTypeParquetIO.SingleNullableDecima…

df284d9

…lColumn

wgtmac requested a review from wjones127 January 10, 2023 15:53

wjones127 mentioned this pull request Jan 10, 2023

[C++][Parquet] Support Decimal from Int32/Int64 in StatisticsAsScalars #19923

Open

wjones127 requested changes Jan 10, 2023

View reviewed changes

cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated Show resolved Hide resolved

liukun4515 reviewed Jan 11, 2023

View reviewed changes

fix store_schema for decimal256

f7ee8e9

wgtmac requested a review from wjones127 January 11, 2023 05:42

wjones127 approved these changes Jan 11, 2023

View reviewed changes

wjones127 merged commit 7d17a5b into apache:master Jan 11, 2023

Dream-hu mentioned this pull request Jan 13, 2023

[Parquet Decimal] Do we has any plan to support short decimal layout, such as decimal64? #33650

Open

pitrou reviewed Jan 16, 2023

View reviewed changes

wjones127 mentioned this pull request Jan 19, 2023

MINOR: [C++][Parquet] Rephrase decimal annotation #33694

Merged

zmanji mentioned this pull request Nov 26, 2023

[Python][Parquet] Support wiriting decimals as integers #38882

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64 #15244

GH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64 #15244

wgtmac commented Jan 7, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 7, 2023

wgtmac commented Jan 8, 2023

wjones127 Jan 9, 2023

emkornfield Jan 9, 2023

wjones127 Jan 9, 2023

emkornfield Jan 13, 2023

pitrou Jan 16, 2023

pitrou Jan 16, 2023

wjones127 left a comment

wjones127 left a comment

liukun4515 Jan 11, 2023

wjones127 Jan 11, 2023

wgtmac Jan 11, 2023

liukun4515 Jan 11, 2023

wgtmac commented Jan 11, 2023

wjones127 left a comment

ursabot commented Jan 12, 2023

ursabot commented Jan 12, 2023

pitrou Jan 16, 2023

pitrou Jan 16, 2023

mapleFU commented Mar 14, 2023

wgtmac commented Mar 14, 2023

mapleFU commented Mar 14, 2023

GH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64 #15244

GH-15239: [C++][Parquet] Parquet writer writes decimal as int32/64 #15244

Conversation

wgtmac commented Jan 7, 2023 • edited by github-actions bot Loading

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 7, 2023

wgtmac commented Jan 8, 2023

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

wjones127 left a comment

Choose a reason for hiding this comment

wjones127 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

wgtmac commented Jan 11, 2023

wjones127 left a comment

Choose a reason for hiding this comment

ursabot commented Jan 12, 2023

ursabot commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Mar 14, 2023

wgtmac commented Mar 14, 2023

mapleFU commented Mar 14, 2023

wgtmac commented Jan 7, 2023 •

edited by github-actions bot

Loading