laskoviymishka commented on code in PR #2009:
URL: https://github.com/apache/iceberg-go/pull/2009#discussion_r4007799137
##########
table/evaluators.go:
##########
@@ -790,6 +792,24 @@ type inclusiveMetricsEval struct {
includeEmptyFiles bool
}
+// intBackedDecimal reports whether a column's statistics are a decimal that
+// Parquet stores in an INT32 or INT64. Parquet's plain encoding for those is
+// little-endian, while an Iceberg bound is big-endian two's complement, so the
+// stat bytes have to be reversed before they can be used as a bound: 659 read
+// as-is would come back as -1828585472 and prune every row group that matches.
+// A FIXED_LEN_BYTE_ARRAY-backed decimal is big-endian in Parquet too, so its
+// bounds are already in Iceberg's form.
+func intBackedDecimal(descr *parquetschema.Column) bool {
Review Comment:
The reversal only lives in `inclusiveMetricsEval`, but `strictMetricsEval`
shares the same `lowerBounds`/`upperBounds` embedding. Nothing's broken today
since strict has no Parquet-stats path, but if one ever populates those maps
from raw Parquet stats (strict row-group pruning, DV stats loading) it silently
hits the identical wrong-bounds bug: zero rows, no error.
I'd either pull the reverse-if-int-backed step into a small helper both
sides call, or drop a note on `intBackedDecimal` that any Parquet-stats-based
bound population has to run through it. wdyt?
##########
table/evaluators.go:
##########
@@ -790,6 +792,24 @@ type inclusiveMetricsEval struct {
includeEmptyFiles bool
}
+// intBackedDecimal reports whether a column's statistics are a decimal that
+// Parquet stores in an INT32 or INT64. Parquet's plain encoding for those is
+// little-endian, while an Iceberg bound is big-endian two's complement, so the
+// stat bytes have to be reversed before they can be used as a bound: 659 read
+// as-is would come back as -1828585472 and prune every row group that matches.
Review Comment:
The doc example works from 659, but the tests all drive -659, so anyone
cross-checking the comment against the test spends a minute reconciling the
sign. I'd swap the example to the -659 the tests actually use, or just drop the
concrete number.
##########
table/evaluators.go:
##########
@@ -843,8 +863,19 @@ func (m *inclusiveMetricsEval) TestRowGroup(rgmeta
*metadata.RowGroupMetaData, c
m.lowerBounds = make(map[int][]byte,
len(colIndices))
m.upperBounds = make(map[int][]byte,
len(colIndices))
}
- m.lowerBounds[fieldID] = stats.EncodeMin()
- m.upperBounds[fieldID] = stats.EncodeMax()
+
+ lower, upper := stats.EncodeMin(), stats.EncodeMax()
+ if intBackedDecimal(stats.Descr()) {
+ // EncodeMin/EncodeMax hand back a freshly
allocated buffer, so
+ // reversing in place cannot disturb the row
group metadata.
+ // Iceberg does not require the minimum number
of bytes, so the
Review Comment:
This reads backwards from the spec. Appendix D does require decimal bounds
be serialized at minimum width, and our own `MarshalBinary` doc says the same,
so "Iceberg does not require the minimum number of bytes" would mislead someone
writing bounds on the manifest path.
What's actually true here is narrower: the decoder (`UnmarshalBinary`) is
width-tolerant, so a fixed 4/8-byte input decodes fine. I'd reword to frame it
as a decoder property rather than a spec relaxation.
##########
table/evaluators_row_group_test.go:
##########
@@ -76,6 +79,218 @@ func buildRowGroupMetricsMetadata(t testing.TB, rowGroups,
columns int, withStat
return meta
}
+// buildDecimalRowGroupMetadata builds a single row group holding one decimal
+// column with the supplied plain-encoded min and max statistics. The min and
+// max are kept distinct so that confusing the lower bound with the upper one
is
+// detectable.
+func buildDecimalRowGroupMetadata(t testing.TB, physical parquet.Type, typeLen
int, precision, scale int32, minEnc, maxEnc []byte) *metadata.FileMetaData {
+ t.Helper()
+ node, err := parquetschema.NewPrimitiveNodeLogical("decimal",
parquet.Repetitions.Required,
+ parquetschema.NewDecimalLogicalType(precision, scale),
physical, typeLen, 1)
+ require.NoError(t, err)
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required,
+ parquetschema.FieldList{node}, -1)
+ require.NoError(t, err)
+
+ size := int64(len(minEnc) + len(maxEnc))
+ builder :=
metadata.NewFileMetadataBuilder(parquetschema.NewSchema(root),
parquet.NewWriterProperties(), nil)
+ rg := builder.AppendRowGroup()
+ rg.SetNumRows(2)
+ chunk := rg.NextColumnChunk()
+ var stats metadata.EncodedStatistics
+ stats.SetMin(minEnc)
+ stats.SetMax(maxEnc)
+ stats.SetNullCount(0)
+ chunk.SetStats(stats)
+ require.NoError(t, chunk.Finish(metadata.ChunkMetaInfo{
+ NumValues: 2,
+ DataPageOffset: 100,
+ IndexPageOffset: -1,
+ CompressedSize: size,
+ UncompressedSize: size,
+ }, false, false, metadata.EncodingStats{}))
+ require.NoError(t, rg.Finish(size, 0))
+
+ meta, err := builder.Finish()
+ require.NoError(t, err)
+
+ return meta
+}
+
+func testDecimalRowGroup(t *testing.T, meta *metadata.FileMetaData, field
iceberg.Type, pred iceberg.BooleanExpression) bool {
+ t.Helper()
+ schema := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "decimal", Type: field, Required: true,
+ })
+ expr, err := iceberg.BindExpr(schema, pred, true)
+ require.NoError(t, err)
+ eval := &inclusiveMetricsEval{expr: expr}
+ keep, err := eval.TestRowGroup(meta.RowGroup(0), []int{0})
+ require.NoError(t, err)
+
+ return keep
+}
+
+// decimalOf builds a decimal predicate value from an unscaled value at the
+// given scale. Named to avoid shadowing by the package's many "dec" locals.
+func decimalOf(unscaled int64, scale int) iceberg.Decimal {
+ return iceberg.Decimal{Val: decimal128.FromI64(unscaled), Scale: scale}
+}
+
+// Parquet writes INT32/INT64-backed decimal statistics little-endian, while an
+// Iceberg bound is big-endian two's complement. Decoding the raw stat bytes as
+// an Iceberg bound therefore yields a wildly wrong value and prunes row groups
+// that do match, so those bounds are byte-reversed first. A
+// FIXED_LEN_BYTE_ARRAY-backed decimal is big-endian in Parquet's plain
encoding
+// too, so it needs no conversion. Each case asserts in both directions: a row
+// group that can match must survive, and one that cannot must be pruned, so
+// neither a corrupted bound nor a silently dropped one passes.
+// See apache/iceberg-go#1876.
+func TestInclusiveMetricsEvalIntBackedDecimalRowGroup(t *testing.T) {
+ // Unscaled bounds of the synthetic row group: -6.59 through 123.45 at
+ // scale 2. The negative minimum exercises the sign-bit decode path.
+ const (
+ minUnscaled int64 = -659
+ maxUnscaled int64 = 12345
Review Comment:
Every case here is mixed-sign, min negative and max positive, so the max
bound never exercises the sign-bit decode after reversal (`maxUnscaled=12345`
reverses to a leading `0x00`). I'd add one case with both bounds negative, say
-12345 and -659, asserting `keep(EqualTo(-700))` and `!keep(EqualTo(-13000))`,
so both bounds get their high bit set. Cheap, and it closes the gap on the
upper bound.
##########
table/evaluators_row_group_test.go:
##########
@@ -76,6 +79,218 @@ func buildRowGroupMetricsMetadata(t testing.TB, rowGroups,
columns int, withStat
return meta
}
+// buildDecimalRowGroupMetadata builds a single row group holding one decimal
+// column with the supplied plain-encoded min and max statistics. The min and
+// max are kept distinct so that confusing the lower bound with the upper one
is
+// detectable.
+func buildDecimalRowGroupMetadata(t testing.TB, physical parquet.Type, typeLen
int, precision, scale int32, minEnc, maxEnc []byte) *metadata.FileMetaData {
+ t.Helper()
+ node, err := parquetschema.NewPrimitiveNodeLogical("decimal",
parquet.Repetitions.Required,
+ parquetschema.NewDecimalLogicalType(precision, scale),
physical, typeLen, 1)
+ require.NoError(t, err)
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required,
+ parquetschema.FieldList{node}, -1)
+ require.NoError(t, err)
+
+ size := int64(len(minEnc) + len(maxEnc))
+ builder :=
metadata.NewFileMetadataBuilder(parquetschema.NewSchema(root),
parquet.NewWriterProperties(), nil)
+ rg := builder.AppendRowGroup()
+ rg.SetNumRows(2)
+ chunk := rg.NextColumnChunk()
+ var stats metadata.EncodedStatistics
+ stats.SetMin(minEnc)
+ stats.SetMax(maxEnc)
+ stats.SetNullCount(0)
+ chunk.SetStats(stats)
+ require.NoError(t, chunk.Finish(metadata.ChunkMetaInfo{
+ NumValues: 2,
+ DataPageOffset: 100,
+ IndexPageOffset: -1,
+ CompressedSize: size,
+ UncompressedSize: size,
+ }, false, false, metadata.EncodingStats{}))
+ require.NoError(t, rg.Finish(size, 0))
+
+ meta, err := builder.Finish()
+ require.NoError(t, err)
+
+ return meta
+}
+
+func testDecimalRowGroup(t *testing.T, meta *metadata.FileMetaData, field
iceberg.Type, pred iceberg.BooleanExpression) bool {
+ t.Helper()
+ schema := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "decimal", Type: field, Required: true,
+ })
+ expr, err := iceberg.BindExpr(schema, pred, true)
+ require.NoError(t, err)
+ eval := &inclusiveMetricsEval{expr: expr}
+ keep, err := eval.TestRowGroup(meta.RowGroup(0), []int{0})
+ require.NoError(t, err)
+
+ return keep
+}
+
+// decimalOf builds a decimal predicate value from an unscaled value at the
+// given scale. Named to avoid shadowing by the package's many "dec" locals.
+func decimalOf(unscaled int64, scale int) iceberg.Decimal {
+ return iceberg.Decimal{Val: decimal128.FromI64(unscaled), Scale: scale}
+}
+
+// Parquet writes INT32/INT64-backed decimal statistics little-endian, while an
+// Iceberg bound is big-endian two's complement. Decoding the raw stat bytes as
+// an Iceberg bound therefore yields a wildly wrong value and prunes row groups
+// that do match, so those bounds are byte-reversed first. A
+// FIXED_LEN_BYTE_ARRAY-backed decimal is big-endian in Parquet's plain
encoding
+// too, so it needs no conversion. Each case asserts in both directions: a row
+// group that can match must survive, and one that cannot must be pruned, so
+// neither a corrupted bound nor a silently dropped one passes.
+// See apache/iceberg-go#1876.
+func TestInclusiveMetricsEvalIntBackedDecimalRowGroup(t *testing.T) {
+ // Unscaled bounds of the synthetic row group: -6.59 through 123.45 at
+ // scale 2. The negative minimum exercises the sign-bit decode path.
+ const (
+ minUnscaled int64 = -659
+ maxUnscaled int64 = 12345
+ scale = 2
+ )
+
+ // Big-endian two's complement, 16 bytes, as Parquet stores a
+ // FIXED_LEN_BYTE_ARRAY decimal and as Iceberg expects a bound.
+ flbaMin := bytes.Repeat([]byte{0xff}, 16)
+ flbaMin[14], flbaMin[15] = 0xfd, 0x6d
+ flbaMax := make([]byte, 16)
+ flbaMax[14], flbaMax[15] = 0x30, 0x39
+
+ tests := []struct {
+ name string
+ physical parquet.Type
+ typeLen int
+ precision int
+ minEnc, maxEnc []byte
+ }{
+ {
+ name: "INT32-backed decimal",
+ physical: parquet.Types.Int32, typeLen: -1, precision:
9,
+ minEnc: []byte{0x6d, 0xfd, 0xff, 0xff}, // -659,
little-endian
+ maxEnc: []byte{0x39, 0x30, 0x00, 0x00}, // 12345,
little-endian
+ },
+ {
+ name: "INT64-backed decimal",
+ physical: parquet.Types.Int64, typeLen: -1, precision:
18,
+ minEnc: []byte{0x6d, 0xfd, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff},
+ maxEnc: []byte{0x39, 0x30, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00},
+ },
+ {
+ // Control: already big-endian, so reversing it would
break it.
+ name: "FIXED_LEN_BYTE_ARRAY decimal",
+ physical: parquet.Types.FixedLenByteArray, typeLen: 16,
precision: 38,
+ minEnc: flbaMin, maxEnc: flbaMax,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ meta := buildDecimalRowGroupMetadata(t, tt.physical,
tt.typeLen,
+ int32(tt.precision), scale, tt.minEnc,
tt.maxEnc)
+ field := iceberg.DecimalTypeOf(tt.precision, scale)
+ ref := iceberg.Reference("decimal")
+
+ keep := func(pred iceberg.BooleanExpression) bool {
+ return testDecimalRowGroup(t, meta, field, pred)
+ }
+
+ // One positive and one negative assertion per bound.
The
+ // "keeps" catch a bound decoded wrongly or swapped
with its
+ // partner; the "prunes" catch a bound dropped rather
than
+ // converted, which would silently disable decimal
pruning.
+ assert.True(t, keep(iceberg.EqualTo(ref,
decimalOf(minUnscaled, scale))),
+ "pruned a row group whose minimum matches")
+ assert.True(t, keep(iceberg.EqualTo(ref,
decimalOf(maxUnscaled, scale))),
+ "pruned a row group whose maximum matches")
+ assert.False(t, keep(iceberg.LessThan(ref,
decimalOf(minUnscaled, scale))),
+ "failed to prune using the lower bound")
+ assert.False(t, keep(iceberg.GreaterThan(ref,
decimalOf(maxUnscaled, scale))),
+ "failed to prune using the upper bound")
+
+ // Equality one step outside each bound. Redundant
against every
+ // regression we could think of, kept as cheap
insurance: equality
+ // just past a bound is the most common real query
shape, and an
+ // off-by-one there would otherwise rest on
LessThan/GreaterThan
+ // alone.
+ assert.False(t, keep(iceberg.EqualTo(ref,
decimalOf(minUnscaled-1, scale))),
+ "failed to prune a value one below the lower
bound")
+ assert.False(t, keep(iceberg.EqualTo(ref,
decimalOf(maxUnscaled+1, scale))),
+ "failed to prune a value one above the upper
bound")
+ })
+ }
+}
+
+// TestInclusiveMetricsEvalRealParquetDecimalRowGroup covers the same ground
+// without hand-written statistics: it writes a real Parquet file with an
+// INT32-backed decimal column and prunes against the metadata the writer
+// produced, so the fixture above cannot drift from what Parquet actually
emits.
+func TestInclusiveMetricsEvalRealParquetDecimalRowGroup(t *testing.T) {
+ const (
+ precision, scale = 9, 2
+ minUnscaled = -659
+ maxUnscaled = 12345
+ )
+
+ node, err := parquetschema.NewPrimitiveNodeLogical("decimal",
parquet.Repetitions.Required,
+ parquetschema.NewDecimalLogicalType(precision, scale),
parquet.Types.Int32, -1, 1)
+ require.NoError(t, err)
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required,
+ parquetschema.FieldList{node}, -1)
+ require.NoError(t, err)
+
+ var buf bytes.Buffer
+ w := file.NewParquetWriter(&buf, root,
+
file.WithWriterProps(parquet.NewWriterProperties(parquet.WithStats(true))))
+ rgw, err := w.AppendRowGroupChecked()
+ require.NoError(t, err)
+ cw, err := rgw.NextColumn()
+ require.NoError(t, err)
+ _, err =
cw.(*file.Int32ColumnChunkWriter).WriteBatch([]int32{minUnscaled, maxUnscaled},
nil, nil)
Review Comment:
This bare assertion panics with a raw stack trace if the writer ever returns
something other than `*file.Int32ColumnChunkWriter`. The rest of this file uses
`require.True` with a `%T` message, which reads better on failure:
```go
cw32, ok := cw.(*file.Int32ColumnChunkWriter)
require.True(t, ok, "expected *file.Int32ColumnChunkWriter, got %T", cw)
_, err = cw32.WriteBatch([]int32{minUnscaled, maxUnscaled}, nil, nil)
```
##########
table/evaluators_row_group_test.go:
##########
@@ -76,6 +79,218 @@ func buildRowGroupMetricsMetadata(t testing.TB, rowGroups,
columns int, withStat
return meta
}
+// buildDecimalRowGroupMetadata builds a single row group holding one decimal
+// column with the supplied plain-encoded min and max statistics. The min and
+// max are kept distinct so that confusing the lower bound with the upper one
is
+// detectable.
+func buildDecimalRowGroupMetadata(t testing.TB, physical parquet.Type, typeLen
int, precision, scale int32, minEnc, maxEnc []byte) *metadata.FileMetaData {
+ t.Helper()
+ node, err := parquetschema.NewPrimitiveNodeLogical("decimal",
parquet.Repetitions.Required,
+ parquetschema.NewDecimalLogicalType(precision, scale),
physical, typeLen, 1)
+ require.NoError(t, err)
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required,
+ parquetschema.FieldList{node}, -1)
+ require.NoError(t, err)
+
+ size := int64(len(minEnc) + len(maxEnc))
+ builder :=
metadata.NewFileMetadataBuilder(parquetschema.NewSchema(root),
parquet.NewWriterProperties(), nil)
+ rg := builder.AppendRowGroup()
+ rg.SetNumRows(2)
+ chunk := rg.NextColumnChunk()
+ var stats metadata.EncodedStatistics
+ stats.SetMin(minEnc)
+ stats.SetMax(maxEnc)
+ stats.SetNullCount(0)
+ chunk.SetStats(stats)
+ require.NoError(t, chunk.Finish(metadata.ChunkMetaInfo{
+ NumValues: 2,
+ DataPageOffset: 100,
+ IndexPageOffset: -1,
+ CompressedSize: size,
+ UncompressedSize: size,
+ }, false, false, metadata.EncodingStats{}))
+ require.NoError(t, rg.Finish(size, 0))
+
+ meta, err := builder.Finish()
+ require.NoError(t, err)
+
+ return meta
+}
+
+func testDecimalRowGroup(t *testing.T, meta *metadata.FileMetaData, field
iceberg.Type, pred iceberg.BooleanExpression) bool {
+ t.Helper()
+ schema := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "decimal", Type: field, Required: true,
+ })
+ expr, err := iceberg.BindExpr(schema, pred, true)
+ require.NoError(t, err)
+ eval := &inclusiveMetricsEval{expr: expr}
+ keep, err := eval.TestRowGroup(meta.RowGroup(0), []int{0})
+ require.NoError(t, err)
+
+ return keep
+}
+
+// decimalOf builds a decimal predicate value from an unscaled value at the
+// given scale. Named to avoid shadowing by the package's many "dec" locals.
+func decimalOf(unscaled int64, scale int) iceberg.Decimal {
+ return iceberg.Decimal{Val: decimal128.FromI64(unscaled), Scale: scale}
+}
+
+// Parquet writes INT32/INT64-backed decimal statistics little-endian, while an
+// Iceberg bound is big-endian two's complement. Decoding the raw stat bytes as
+// an Iceberg bound therefore yields a wildly wrong value and prunes row groups
+// that do match, so those bounds are byte-reversed first. A
+// FIXED_LEN_BYTE_ARRAY-backed decimal is big-endian in Parquet's plain
encoding
+// too, so it needs no conversion. Each case asserts in both directions: a row
+// group that can match must survive, and one that cannot must be pruned, so
+// neither a corrupted bound nor a silently dropped one passes.
+// See apache/iceberg-go#1876.
+func TestInclusiveMetricsEvalIntBackedDecimalRowGroup(t *testing.T) {
+ // Unscaled bounds of the synthetic row group: -6.59 through 123.45 at
+ // scale 2. The negative minimum exercises the sign-bit decode path.
+ const (
+ minUnscaled int64 = -659
+ maxUnscaled int64 = 12345
+ scale = 2
+ )
+
+ // Big-endian two's complement, 16 bytes, as Parquet stores a
+ // FIXED_LEN_BYTE_ARRAY decimal and as Iceberg expects a bound.
+ flbaMin := bytes.Repeat([]byte{0xff}, 16)
+ flbaMin[14], flbaMin[15] = 0xfd, 0x6d
+ flbaMax := make([]byte, 16)
+ flbaMax[14], flbaMax[15] = 0x30, 0x39
+
+ tests := []struct {
+ name string
+ physical parquet.Type
+ typeLen int
+ precision int
+ minEnc, maxEnc []byte
+ }{
+ {
+ name: "INT32-backed decimal",
+ physical: parquet.Types.Int32, typeLen: -1, precision:
9,
+ minEnc: []byte{0x6d, 0xfd, 0xff, 0xff}, // -659,
little-endian
+ maxEnc: []byte{0x39, 0x30, 0x00, 0x00}, // 12345,
little-endian
+ },
+ {
+ name: "INT64-backed decimal",
+ physical: parquet.Types.Int64, typeLen: -1, precision:
18,
+ minEnc: []byte{0x6d, 0xfd, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff},
+ maxEnc: []byte{0x39, 0x30, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00},
+ },
+ {
+ // Control: already big-endian, so reversing it would
break it.
+ name: "FIXED_LEN_BYTE_ARRAY decimal",
+ physical: parquet.Types.FixedLenByteArray, typeLen: 16,
precision: 38,
+ minEnc: flbaMin, maxEnc: flbaMax,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ meta := buildDecimalRowGroupMetadata(t, tt.physical,
tt.typeLen,
+ int32(tt.precision), scale, tt.minEnc,
tt.maxEnc)
+ field := iceberg.DecimalTypeOf(tt.precision, scale)
+ ref := iceberg.Reference("decimal")
+
+ keep := func(pred iceberg.BooleanExpression) bool {
+ return testDecimalRowGroup(t, meta, field, pred)
+ }
+
+ // One positive and one negative assertion per bound.
The
+ // "keeps" catch a bound decoded wrongly or swapped
with its
+ // partner; the "prunes" catch a bound dropped rather
than
+ // converted, which would silently disable decimal
pruning.
+ assert.True(t, keep(iceberg.EqualTo(ref,
decimalOf(minUnscaled, scale))),
+ "pruned a row group whose minimum matches")
+ assert.True(t, keep(iceberg.EqualTo(ref,
decimalOf(maxUnscaled, scale))),
+ "pruned a row group whose maximum matches")
+ assert.False(t, keep(iceberg.LessThan(ref,
decimalOf(minUnscaled, scale))),
+ "failed to prune using the lower bound")
+ assert.False(t, keep(iceberg.GreaterThan(ref,
decimalOf(maxUnscaled, scale))),
+ "failed to prune using the upper bound")
+
+ // Equality one step outside each bound. Redundant
against every
+ // regression we could think of, kept as cheap
insurance: equality
+ // just past a bound is the most common real query
shape, and an
+ // off-by-one there would otherwise rest on
LessThan/GreaterThan
+ // alone.
+ assert.False(t, keep(iceberg.EqualTo(ref,
decimalOf(minUnscaled-1, scale))),
+ "failed to prune a value one below the lower
bound")
+ assert.False(t, keep(iceberg.EqualTo(ref,
decimalOf(maxUnscaled+1, scale))),
+ "failed to prune a value one above the upper
bound")
+ })
+ }
+}
+
+// TestInclusiveMetricsEvalRealParquetDecimalRowGroup covers the same ground
+// without hand-written statistics: it writes a real Parquet file with an
+// INT32-backed decimal column and prunes against the metadata the writer
+// produced, so the fixture above cannot drift from what Parquet actually
emits.
+func TestInclusiveMetricsEvalRealParquetDecimalRowGroup(t *testing.T) {
+ const (
+ precision, scale = 9, 2
+ minUnscaled = -659
+ maxUnscaled = 12345
+ )
+
+ node, err := parquetschema.NewPrimitiveNodeLogical("decimal",
parquet.Repetitions.Required,
+ parquetschema.NewDecimalLogicalType(precision, scale),
parquet.Types.Int32, -1, 1)
+ require.NoError(t, err)
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required,
+ parquetschema.FieldList{node}, -1)
+ require.NoError(t, err)
+
+ var buf bytes.Buffer
+ w := file.NewParquetWriter(&buf, root,
+
file.WithWriterProps(parquet.NewWriterProperties(parquet.WithStats(true))))
+ rgw, err := w.AppendRowGroupChecked()
+ require.NoError(t, err)
+ cw, err := rgw.NextColumn()
+ require.NoError(t, err)
+ _, err =
cw.(*file.Int32ColumnChunkWriter).WriteBatch([]int32{minUnscaled, maxUnscaled},
nil, nil)
+ require.NoError(t, err)
+ require.NoError(t, cw.Close())
+ require.NoError(t, rgw.Close())
+ require.NoError(t, w.Close())
+
+ rdr, err := file.NewParquetReader(bytes.NewReader(buf.Bytes()))
+ require.NoError(t, err)
+ t.Cleanup(func() { require.NoError(t, rdr.Close()) })
+
+ meta := rdr.MetaData()
+ chunk, err := meta.RowGroup(0).ColumnChunk(0)
+ require.NoError(t, err)
+ stats, err := chunk.Statistics()
+ require.NoError(t, err)
+ require.True(t, intBackedDecimal(stats.Descr()),
+ "writer did not produce an INT32-backed decimal, so this test
no longer covers the bug")
+
+ field := iceberg.DecimalTypeOf(precision, scale)
+ ref := iceberg.Reference("decimal")
+ schema := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "decimal", Type: field, Required: true,
+ })
+
+ keep := func(pred iceberg.BooleanExpression) bool {
+ expr, err := iceberg.BindExpr(schema, pred, true)
+ require.NoError(t, err)
+ eval := &inclusiveMetricsEval{expr: expr}
+ result, err := eval.TestRowGroup(meta.RowGroup(0), []int{0})
+ require.NoError(t, err)
+
+ return result
+ }
+
+ assert.True(t, keep(iceberg.EqualTo(ref, decimalOf(minUnscaled,
scale))),
Review Comment:
This asserts a lower-bound keep and an upper-bound prune, but not the
reverse pair, so if the two corrected bounds got swapped in the real-writer
path it would slip through here (the synthetic test would still catch it, but
this is the one validating against what Parquet actually emits). I'd add
`EqualTo(maxUnscaled)` keep and `LessThan(minUnscaled)` prune to make it
symmetric. Optional.
--
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]
