The decimal data type is transformed after the data is inserted.

[as10128]

Apache Iceberg version

0.6.0 (latest release)

Please describe the bug 🐞

Version: Pyiceberg 0.6.1

I create a table, there are multiple columns of type decimal, decimal(32,16), decimal(32,2),
after I build the table successfully, the metadata.json file shows this,

After using the append method to insert data, the columns of decimal data type all become decimal(32,2).

I continue to insert data, and an error is reported. I still want to use the schema I created as the schema for my iceber table, how do I do that?

[ndrluis]

Hello @as10128, can you share with us a minimal code example that reproduces this error?

[as10128]

Hello @ndrluis ,I wrote a test code, I used minio and hive catalog, you need to modify the aws_access_key, aws_secret_key, s3_endpoint, hive_thrift_uri, warehouse_uri parameters.The results are annotated in the code.
Python 3.10.14
MacOS x86

from pyiceberg.catalog import load_catalog
from pyiceberg.schema import Schema
from pyiceberg.types import (
    DecimalType,
    LongType,
    NestedField,
)
from pyiceberg.exceptions import NoSuchTableError
import pyarrow as pa
from decimal import Decimal

aws_access_key = "aws_access_key"
aws_secret_key = "aws_secret_key"
s3_endpoint = "http://s3_ip:9000"
hive_thrift_uri = "thrift://hive_metastore_ip:9083"
warehouse_uri = "s3a://xxxx/xxxx"


catalog = load_catalog("default", **{
    "uri": f"{hive_thrift_uri}",
    "s3.endpoint": f"{s3_endpoint}",
    "py-io-impl": "pyiceberg.io.pyarrow.PyArrowFileIO",
    "s3.access-key-id": f"{aws_access_key}",
    "s3.secret-access-key": f"{aws_secret_key}",
})

try:
    catalog.drop_table(identifier='cw.decimal_type_table')
except NoSuchTableError:
    pass

data = [
    {"intertype": 1, "decimaltype1": Decimal('52.30'), "decimaltype2": Decimal('52.30')},
    {"intertype": 2, "decimaltype1": Decimal('52.30'), "decimaltype2": Decimal('52.30')},
    {"intertype": 3, "decimaltype1": Decimal('52.30'), "decimaltype2": Decimal('52.30')},
]
arrow_schema = pa.schema(
    [
        pa.field("intertype", pa.int64(), nullable=False),
        pa.field("decimaltype1", pa.decimal128(32, 16), nullable=True),
        pa.field("decimaltype2", pa.decimal128(32, 2), nullable=True),
    ]
)
iceberg_schema = Schema(
    NestedField(field_id=1, name="intertype", field_type=LongType(), required=True),
    NestedField(field_id=2, name="decimaltype1", field_type=DecimalType(32, 16), required=False),
    NestedField(field_id=3, name="decimaltype2", field_type=DecimalType(32, 2), required=False),
    identifier_field_ids=[1]
)
if __name__ == '__main__':
    create_table_instance = catalog.create_table(identifier='cw.decimal_type_table', schema=iceberg_schema)
    print(create_table_instance)
    # decimal_type_table(
    #     1: intertype: required long,
    #     2: decimaltype1: optional decimal(32, 16),
    #     3: decimaltype2: optional decimal(32, 2)
    # ),
    # partition by: [],
    # sort order: [],
    # snapshot: null

    assert (create_table_instance.schema().columns[1] ==
            NestedField(field_id=2, name="decimaltype1", field_type=DecimalType(32, 16), required=False))

    # insert data
    df = pa.Table.from_pylist(data, schema=arrow_schema)
    create_table_instance.append(df)


    load_table_instance = catalog.load_table('cw.decimal_type_table')
    print(load_table_instance)
    # decimal_type_table(
    #     1: intertype: required long,
    #     2: decimaltype1: optional decimal(32, 2),
    #     3: decimaltype2: optional decimal(32, 2)
    # ),
    # partition by: [],
    # sort order: [],
    # snapshot: Operation.APPEND: id = 4966508564674566147, schema_id = 0
    assert (load_table_instance.schema().columns[1] ==
            NestedField(field_id=2, name="decimaltype1", field_type=DecimalType(32, 16), required=False))

[ndrluis]

The problem is not with append; it is something occurring when we call model_copy(deep=True) on the metadata object.

When I was debugging, I couldn't find where the update was occurring. So, I did a sanity check by creating a table and then calling table.metadata.model_copy(deep=True), and then I was able to reproduce the error.

def test_schema_mutation(catalog: SqlCatalog, table_schema_decimal: Schema, random_identifier: Identifier) -> None:
    database_name, _table_name = random_identifier
    catalog.create_namespace(database_name)
    table = catalog.create_table(random_identifier, table_schema_decimal)

    schema = table.schema()

    schema_copy = table.metadata.model_copy(deep=True).schemas[0]

    assert schema == schema_copy

Where table_schema_decimal is:

Schema(
    NestedField(field_id=1, name="integer", field_type=IntegerType(), required=True),
    NestedField(field_id=2, name="decimal_32_2", field_type=DecimalType(32, 2), required=False),
    NestedField(field_id=3, name="decimal_32_16", field_type=DecimalType(32, 16), required=False),
    schema_id=0,
    identifier_field_ids=[1],
)

I will continue trying to understand what is happening, but I don't know if some other contributor can help me to understand where the problem is.

[ndrluis]

I have discovered the problem, but I don't know how to solve it. I removed the Singleton inheritance from the PrimitiveType class and this solved the problem.

While debugging, I discovered that when the model_copy runs, the DecimalType in the Singleton initializer does not receive any arguments; it's an empty tuple (https://github.com/apache/iceberg-python/blob/main/pyiceberg/utils/singleton.py#L45). I believe this is the problem.

@Fokko, can you help with a suggestion to solve this?

[kevinjqliu]

Added #838 to reproduce the bug above.

This is affecting both FixedType and DecimalType; and possibly other classes that utilize Singleton.
I think this is related to how the Singleton class creates and caches classes.

Since new_metadata.model_copy(deep=True) is used to update table metadata, this bug affects any Pydantic model within TableMetadata.

[kevinjqliu]

As a workaround, removing Singleton from PrimitiveType resolves the issue.

iceberg-python/pyiceberg/types.py

Line 163 in a29491a

class PrimitiveType(IcebergRootModel[str], IcebergType, Singleton):

[kevinjqliu]

On a meta-level, does it make sense for FixedType to be a Singleton? FixedType(8) and FixedType(16) are different data types.

We can possibly introduce a new class ComplexType, which does not use the Singleton class, to deal with FixedType and DecimalType

[kevinjqliu]

@Fokko please take a look at this issue when you get a chance. It has to do with data type correctness.

[sungwy]

Just copying this test from the Monthly discussion:

from pyiceberg.types import FixedType
from copy import deepcopy

deepcopy([FixedType(8), FixedType(16)])

yields:

[FixedType(length=16), FixedType(length=16)]

The issue seems to be with the inability to deepcopy the Singleton types correctly, wondering if there's a way to force deepcopy to force initialization of each of the FixedTypes in the object with separate arguments

[ndrluis]

@syun64 when debugging or printing the key variable in the Singleton class you will see that when we initialize the class we have the arguments, but when the deepcopy occurs we don't have.

Example with printing the arguments

(<class 'pyiceberg.types.FixedType'>, (8,), ()) <- Initialization
(<class 'pyiceberg.types.FixedType'>, (16,), ()) <- Initialization
(<class 'pyiceberg.types.FixedType'>, (), ()) <- deep copy
(<class 'pyiceberg.types.FixedType'>, (), ()) <- deep copy

printing the instances

key (<class 'pyiceberg.types.FixedType'>, (8,), ())
instances {}
key (<class 'pyiceberg.types.FixedType'>, (16,), ())
instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8)}
key (<class 'pyiceberg.types.FixedType'>, (), ())
instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8), (<class 'pyiceberg.types.FixedType'>, (16,), ()): FixedType(length=16)}
key (<class 'pyiceberg.types.FixedType'>, (), ())
instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8), (<class 'pyiceberg.types.FixedType'>, (16,), ()): FixedType(length=16), (<class 'pyiceberg.types.FixedType'>, (), ()): FixedType(length=8)}

[sungwy]

@syun64 when debugging or printing the key variable in the Singleton class you will see that when we initialize the class we have the arguments, but when the deepcopy occurs we don't have.

Example with printing the arguments

(<class 'pyiceberg.types.FixedType'>, (8,), ()) <- Initialization

(<class 'pyiceberg.types.FixedType'>, (16,), ()) <- Initialization

(<class 'pyiceberg.types.FixedType'>, (), ()) <- deep copy

(<class 'pyiceberg.types.FixedType'>, (), ()) <- deep copy

printing the instances

key (<class 'pyiceberg.types.FixedType'>, (8,), ())

instances {}

key (<class 'pyiceberg.types.FixedType'>, (16,), ())

instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8)}

key (<class 'pyiceberg.types.FixedType'>, (), ())

instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8), (<class 'pyiceberg.types.FixedType'>, (16,), ()): FixedType(length=16)}

key (<class 'pyiceberg.types.FixedType'>, (), ())

instances {(<class 'pyiceberg.types.FixedType'>, (8,), ()): FixedType(length=8), (<class 'pyiceberg.types.FixedType'>, (16,), ()): FixedType(length=16), (<class 'pyiceberg.types.FixedType'>, (), ()): FixedType(length=8)}

This is so helpful @ndrluis - thank you for sharing this! My knowledge of how deep copy exactly works is a bit limited, but I wonder if we can override this behavior somehow. If that's not possible, I think we will have to consider one of the two following options:

1. Find a way to selectively avoid deep copying (and instead just copy) certain classes - since the Singleton classes cannot be deep copied anyways, we should be okay keeping the previous object reference on the deep copied TableMetadata
2. As @kevinjqliu already suggested, don't inherit from the Singleton class for these multi argument classes.

My preference is for option (1) - would it be possible to override deepcopy of the Singleton class to invoke copy instead?

[ndrluis]

@syun64 I believe that I have discovered how to solve the problem. I will open a PR, but I need some double-checking to ensure my verification is correct. This is the code that I'm using to verify it.

first = [FixedType(8), FixedType(16)]
copied = deepcopy(first)

print("list id", id(first))
print("copied id", id(copied))

print("list id must be different", id(first) != id(copied))

print(id(first[0]) == id(copied[0]))
print(id(first[1]) == id(copied[1]))

[kevinjqliu]

That makes sense to me.
Specifically, items in the original list should have the same ids as items in the new deep-copied list.

assert id(first[0]) == id(copied[0])
assert id(first[1]) == id(copied[1])