laskoviymishka commented on code in PR #931:
URL: https://github.com/apache/iceberg-go/pull/931#discussion_r3712143958
##########
table/internal/parquet_files.go:
##########
@@ -314,16 +320,36 @@ func (parquetFormat) GetWriteProperties(props
iceberg.Properties) any {
if !ok || colName == "" {
continue
}
- // EqualFold matches Java's Boolean.parseBoolean: only "true"
- // (case-insensitive) is true; anything else, including "1" or
- // "yes", is false. strconv.ParseBool behaves differently ("1"
→ true).
- enabled := strings.EqualFold(val, "true")
- writerProps = append(writerProps,
parquet.WithBloomFilterEnabledFor(colName, enabled))
+ writerProps = append(writerProps,
parquet.WithBloomFilterEnabledFor(colName, javaBool(val)))
+ }
+
+ // Dictionary encoding is on unless parquet.enable.dictionary disables
it;
+ // write.parquet.dict-encoding-enabled.column.<col-name> overrides per
column.
+ dictEnabled := ParquetDictEnabledDefault
+ if val, ok := props[ParquetDictEnabledKey]; ok {
+ dictEnabled = javaBool(val)
+ }
+ writerProps = append(writerProps,
parquet.WithDictionaryDefault(dictEnabled))
+
+ prefix = ParquetDictEncodingColumnEnabledKeyPrefix + "."
+ for key, val := range props {
+ colName, ok := strings.CutPrefix(key, prefix)
+ if !ok || colName == "" {
+ continue
+ }
+ writerProps = append(writerProps,
parquet.WithDictionaryFor(colName, javaBool(val)))
Review Comment:
The suffix here is an Iceberg field name, but `WithDictionaryFor` matches on
the Parquet physical column path. For flat top-level columns those are the
same, which is why the tests pass, but for a nested field they diverge
(`items.element` in Iceberg is `items.list.element` in Parquet) and a
per-column opt-out silently does nothing.
The bloom-filter loop above does the same thing, so I don't think this PR
needs to fix the translation. I'd just add a short comment noting the suffix is
treated as a Parquet path, so nobody sets it on a list column and wonders why
nothing changed. Java maps it through `colNameToParquetPathMap` if we ever want
to close it properly. wdyt?
##########
table/internal/parquet_files_test.go:
##########
@@ -1247,10 +1247,252 @@ func TestGetWritePropertiesPageVersion(t *testing.T) {
pageRdr, err := rdr.RowGroup(0).GetColumnPageReader(0)
require.NoError(t, err)
- require.True(t, pageRdr.Next())
- assert.Equal(t, tt.expectedPageType,
pageRdr.Page().Type())
+ var dataPageType file.PageType
+ var foundDataPage bool
+ for pageRdr.Next() {
+ pt := pageRdr.Page().Type()
+ if pt == file.PageTypeDictionaryPage {
+ continue
+ }
+ dataPageType = pt
+ foundDataPage = true
+
+ break
+ }
+ require.True(t, foundDataPage, "expected a data page in
row group")
+ assert.Equal(t, tt.expectedPageType, dataPageType)
+ })
+ }
+}
+
+func TestGetWritePropertiesDictionaryEnabledByDefault(t *testing.T) {
+ format := internal.GetFileFormat(iceberg.ParquetFile)
+ writeProps :=
format.GetWriteProperties(iceberg.Properties{}).([]parquet.WriterProperty)
+
+ root, err := schema.NewGroupNode("schema",
parquet.Repetitions.Required, schema.FieldList{
+ schema.NewInt32Node("col", parquet.Repetitions.Required, -1),
+ }, -1)
+ require.NoError(t, err)
+
+ var buf bytes.Buffer
+ pw := file.NewParquetWriter(&buf, root, file.WithWriterProps(
+ parquet.NewWriterProperties(writeProps...),
+ ))
+
+ rgw, err := pw.AppendRowGroupChecked()
+ require.NoError(t, err)
+ cw, err := rgw.NextColumn()
+ require.NoError(t, err)
+
+ // Low-cardinality column stays dictionary-encoded (well under the
dict-size limit).
+ values := make([]int32, 1024)
+ for i := range values {
+ values[i] = int32(i % 8)
+ }
+ _, err = cw.(*file.Int32ColumnChunkWriter).WriteBatch(values, nil, nil)
+ require.NoError(t, err)
+ require.NoError(t, cw.Close())
+ require.NoError(t, rgw.Close())
+ require.NoError(t, pw.Close())
+
+ rdr, err := file.NewParquetReader(bytes.NewReader(buf.Bytes()))
+ require.NoError(t, err)
+ defer rdr.Close()
+
+ colMeta, err := rdr.MetaData().RowGroup(0).ColumnChunk(0)
+ require.NoError(t, err)
+
+ assert.True(t, colMeta.HasDictionaryPage(),
+ "expected a dictionary page with default write properties")
+ assert.Contains(t, colMeta.Encodings(), parquet.Encodings.RLEDict,
+ "expected RLE_DICTIONARY encoding with default write
properties")
+}
+
+func TestGetWritePropertiesDictionaryEncoding(t *testing.T) {
+ format := internal.GetFileFormat(iceberg.ParquetFile)
+ writerProps := func(props iceberg.Properties) *parquet.WriterProperties
{
+ return
parquet.NewWriterProperties(format.GetWriteProperties(props).([]parquet.WriterProperty)...)
+ }
+
+ t.Run("enabled by default", func(t *testing.T) {
+ wp := writerProps(iceberg.Properties{})
+ assert.True(t, wp.DictionaryEnabled())
+ assert.True(t, wp.DictionaryEnabledFor("any_col"))
+ })
+
+ t.Run("global opt-out", func(t *testing.T) {
+ wp :=
writerProps(iceberg.Properties{internal.ParquetDictEnabledKey: "false"})
+ assert.False(t, wp.DictionaryEnabled())
+ assert.False(t, wp.DictionaryEnabledFor("any_col"))
+ })
+
+ t.Run("per-column opt-out under the default", func(t *testing.T) {
+ wp := writerProps(iceberg.Properties{
+ internal.ParquetDictEncodingColumnEnabledKeyPrefix +
".name": "false",
+ })
+ assert.False(t, wp.DictionaryEnabledFor("name"))
+ assert.True(t, wp.DictionaryEnabledFor("id"), "unmentioned
columns follow the global setting")
+ })
+
+ t.Run("per-column opt-in under a global opt-out", func(t *testing.T) {
+ wp := writerProps(iceberg.Properties{
+ internal.ParquetDictEnabledKey:
"false",
+ internal.ParquetDictEncodingColumnEnabledKeyPrefix +
".id": "true",
+ internal.ParquetDictEncodingColumnEnabledKeyPrefix +
".name": "false",
})
+ assert.True(t, wp.DictionaryEnabledFor("id"))
+ assert.False(t, wp.DictionaryEnabledFor("name"))
+ assert.False(t, wp.DictionaryEnabledFor("unmentioned_col"),
"unmentioned columns follow the global setting")
+ })
+
+ // Java reads both keys with Boolean.parseBoolean, so only "true"
enables.
+ t.Run("java boolean semantics", func(t *testing.T) {
+ for _, val := range []string{"true", "TRUE", "True"} {
+ assert.True(t,
writerProps(iceberg.Properties{internal.ParquetDictEnabledKey:
val}).DictionaryEnabled(), val)
+ }
+ for _, val := range []string{"false", "FALSE", "1", "yes", "",
"bogus"} {
+ assert.False(t,
writerProps(iceberg.Properties{internal.ParquetDictEnabledKey:
val}).DictionaryEnabled(), val)
+ }
+ })
+}
+
+func TestGetWritePropertiesDictionaryEncodingRoundTrip(t *testing.T) {
+ values := make([]parquet.ByteArray, 4096)
+ for i := range values {
+ values[i] = parquet.ByteArray(fmt.Sprintf("cat_%02d", i%8))
}
+
+ t.Run("default writes a dictionary", func(t *testing.T) {
+ chunk := writeDictTestColumn(t, iceberg.Properties{}, values)
+ assert.True(t, chunk.hasDictPage)
+ assert.Positive(t, chunk.dictDataPages)
+ assert.Zero(t, chunk.plainDataPages)
+ })
+
+ t.Run("global opt-out writes no dictionary", func(t *testing.T) {
+ chunk := writeDictTestColumn(t, iceberg.Properties{
+ internal.ParquetDictEnabledKey: "false",
+ }, values)
+ assert.False(t, chunk.hasDictPage)
+ assert.Zero(t, chunk.dictDataPages)
+ assert.Positive(t, chunk.plainDataPages)
+ })
+
+ t.Run("per-column opt-out writes no dictionary", func(t *testing.T) {
+ chunk := writeDictTestColumn(t, iceberg.Properties{
+ internal.ParquetDictEncodingColumnEnabledKeyPrefix +
"." + dictTestColumn: "false",
+ }, values)
+ assert.False(t, chunk.hasDictPage)
Review Comment:
The round-trip covers default, global opt-out, and per-column opt-out at the
file level, but per-column opt-in under a global opt-out is only checked on the
WriterProperties object. That's the case the feature exists for, and the
object-level check won't catch arrow-go dropping the per-column flag when the
global is off.
I'd add a sibling subtest here that sets the global off and this column on,
and asserts `hasDictPage` is true.
--
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]
