Parquet reader forces Decimal32/64 columns to Decimal128

[scovich]

Describe the bug

The parquet reader always widens INT32 (Decimal) and INT64 (Decimal) columns to Decimal128. It might be possible for a reader to specifically request the narrower types in their read schema, but if they just read what was written to disk they get back a different type than what was written.

To Reproduce

Several unit tests expect the current behavior, and started failing when I changed the parquet reader to preserve types:

test arrow::array_reader::primitive_array::tests::test_primitive_array_reader_decimal_types ... FAILED
test arrow::arrow_reader::tests::test_decimal ... FAILED
test arrow::arrow_reader::tests::test_arbitrary_decimal ... FAILED
test arrow::arrow_reader::tests::test_read_decimal_file ... FAILED
test arrow::arrow_writer::tests::arrow_writer_decimal128_dictionary ... FAILED
test arrow::arrow_writer::tests::arrow_writer_decimal ... FAILED
test arrow::arrow_writer::tests::arrow_writer_decimal256_dictionary ... FAILED
test arrow::arrow_writer::tests::arrow_writer_decimal64_dictionary ... FAILED
test arrow::schema::tests::test_arrow_schema_roundtrip ... FAILED
test arrow::schema::tests::test_decimal_fields ... FAILED
test arrow::schema::tests::test_column_desc_to_field ... FAILED
test statistics::test_decimal128 ... FAILED
test statistics::test_decimal_256 ... FAILED
test statistics::test_decimal64 ... FAILED
test statistics::test_data_page_stats_with_all_null_page ... FAILED

Expected behavior

If it was written as INT32 (Decimal) it should come back as Decimal32 unless the user requested something else via the read schema. Ditto for INT64 (Decimal) as Decimal64.

Additional context

See #8540 (comment)

The variant shredding decimal integration tests fail by default, because they expect 32- and 64-bit decimals to faithfully round trip through parquet; I had to add a manual casting operation VariantArray constructor to compensate.

For the fix I tried (but eventually reverted in favor of the above-mentioned casting), see
https://github.com/apache/arrow-rs/pull/8540/files/cd978f5374503fff3a265bc06ad6ce143a4110f3..5b55f121d7461bd31dfc2d204f8fc303523a3988

The idea was to split the decimal_type helper (which only understood 128- and 256-bit decimals) into decimal_[32|64|128|256]_type helpers, where each helper's name indicates a lower bound on the resulting decimal's width. Too-high precision can still force a wider decimal, tho it's not clear to me that this is actually correct -- is e.g. INT32 (Decimal(15, 2)) legal?

[tustvold]

This isn't a bug, the new decimal types were only added very recently to the arrow standard, the decimal coercion logic predates this.

Changing this might be sensible but would constitute a breaking change and probably needs to be opt-in.

FWIW I would expect a Decimal32Array, i.e. the arrow version, to use the embedded arrow schema to roundtrip. But I suspect you are referring to roundtripping outside arrow.

Edit: IIRC there may even be some smarts on the write path to use smaller representations when writing Decimal128Array based on the precision, which would further complicate any changes here.

[scovich]

FWIW I would expect a Decimal32Array, i.e. the arrow version, to use the embedded arrow schema to roundtrip. But I suspect you are referring to roundtripping outside arrow.

You're right, the parquet files used by variant shredding integration are written by parquet-mr, not arrow:

File path:  parquet-testing/shredded_variant/case-025.parquet
Created by: parquet-mr version 1.16.0-SNAPSHOT (build ee34713e4d906d61f95d2b09145945638b2e2296)
Properties:
  parquet.avro.schema: {"type":"record","name":"table","fields":[{"name":"id","type":"int"},{"name":"var","type":["null",{"type":"record","name":"var","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":["null","bytes"],"default":null},{"name":"typed_value","type":["null","int"],"default":null}]}],"default":null}]}
    writer.model.name: avro
Schema:
message table {
  required int32 id = 1;
  optional group var (VARIANT(1)) = 2 {
    required binary metadata;
    optional binary value;
    optional int32 typed_value (DECIMAL(9,4));
  }
}


Row group 0:  count: 1  127.00 B records  start: 4  total(compressed): 127 B total(uncompressed):127 B 
--------------------------------------------------------------------------------
                 type      encodings count     avg size   nulls   min / max
id               INT32     _   _     1         27.00 B    0       "1" / "1"
var.metadata     BINARY    _   _     1         36.00 B    0       "0x010000" / "0x010000"
var.value        BINARY    _   _     1         30.00 B    1       
var.typed_value  INT32     _   _     1         34.00 B    0       "-12345.6789" / "-12345.6789"

... tho even lacking arrow info, it seems like an INT32 (Decimal(9, 4)) column really is decimal32?

IIRC there may even be some smarts on the write path to use smaller representations when writing Decimal128Array based on the precision, which would further complicate any changes here.

You mean, the parquet writer might emit an INT32 (Decimal(9, 4)) column when asked to write out a Decimal128(9, 4) column? Or it might emit an INT32 (Decimal(30, 4)) column if it notices that the values all happen to fit?

Either way, I thought the parquet writer's job was to emit what it was told to emit, not get fancy with data types and automatic conversions?? That came up several times in variant shredding discussions...

[tustvold]

See #3431

The TLDR is historically parquet had types without a corresponding arrow type, and working around this required some smarts.