kroushan-nit commented on code in PR #976:
URL: https://github.com/apache/incubator-xtable/pull/976#discussion_r4356647486
##########
xtable-core/src/main/java/org/apache/xtable/paimon/PaimonPartitionExtractor.java:
##########
@@ -62,21 +62,26 @@ public List<InternalPartitionField>
toInternalPartitionFields(
public List<PartitionValue> toPartitionValues(
FileStoreTable table, BinaryRow partition, InternalSchema
internalSchema) {
- InternalRowPartitionComputer partitionComputer =
newPartitionComputer(table);
- Map<String, String> partValues =
partitionComputer.generatePartValues(partition);
-
- if (partValues.isEmpty()) {
- return Collections.emptyList();
- }
-
- List<PartitionValue> partitionValues = new ArrayList<>(partValues.size());
- for (Map.Entry<String, String> entry : partValues.entrySet()) {
- PartitionValue partitionValue =
+ List<String> partitionKeys = table.partitionKeys();
+ List<PartitionValue> partitionValues = new
ArrayList<>(partitionKeys.size());
+ for (int i = 0; i < partitionKeys.size(); i++) {
+ InternalPartitionField partitionField =
+ toPartitionField(partitionKeys.get(i), internalSchema);
+ InternalSchema fieldSchema = partitionField.getSourceField().getSchema();
+ // read the typed value from the partition row, the string form from
+ // InternalRowPartitionComputer is only meant for paths
+ Object value = statsExtractor.getValue(partition, i,
fieldSchema.getDataType(), fieldSchema);
+ if (value == null && !partition.isNullAt(i)) {
+ throw new ReadException(
+ String.format(
+ "Unsupported partition type %s for partition key %s",
+ fieldSchema.getDataType(), partitionKeys.get(i)));
+ }
+ partitionValues.add(
PartitionValue.builder()
- .partitionField(toPartitionField(entry.getKey(), internalSchema))
- .range(Range.scalar(entry.getValue()))
- .build();
- partitionValues.add(partitionValue);
+ .partitionField(partitionField)
+ .range(Range.scalar(value))
Review Comment:
How should TIMESTAMP partition keys behave after this? `getValue` returns
epoch millis for TIMESTAMP(0-3) and micros for higher precision, and neither
target converts identity partition values:
- Delta: `convertToDeltaPartitionValue` only special-cases DATE, so the
partition value becomes something like `1704067200000000` instead of a
timestamp string.
- Iceberg: the target always declares timestamps as micros and passes the
value through as-is, so a TIMESTAMP(3) partition gets millis read as micros and
lands near 1970.
If timestamp partitions aren't meant to be supported yet, it may be safer to
throw for them, like the unsupported-type branch, than to write wrong values.
##########
xtable-core/src/main/java/org/apache/xtable/paimon/PaimonPartitionExtractor.java:
##########
@@ -62,21 +62,26 @@ public List<InternalPartitionField>
toInternalPartitionFields(
public List<PartitionValue> toPartitionValues(
FileStoreTable table, BinaryRow partition, InternalSchema
internalSchema) {
- InternalRowPartitionComputer partitionComputer =
newPartitionComputer(table);
- Map<String, String> partValues =
partitionComputer.generatePartValues(partition);
-
- if (partValues.isEmpty()) {
- return Collections.emptyList();
- }
-
- List<PartitionValue> partitionValues = new ArrayList<>(partValues.size());
- for (Map.Entry<String, String> entry : partValues.entrySet()) {
- PartitionValue partitionValue =
+ List<String> partitionKeys = table.partitionKeys();
+ List<PartitionValue> partitionValues = new
ArrayList<>(partitionKeys.size());
+ for (int i = 0; i < partitionKeys.size(); i++) {
+ InternalPartitionField partitionField =
+ toPartitionField(partitionKeys.get(i), internalSchema);
+ InternalSchema fieldSchema = partitionField.getSourceField().getSchema();
+ // read the typed value from the partition row, the string form from
+ // InternalRowPartitionComputer is only meant for paths
+ Object value = statsExtractor.getValue(partition, i,
fieldSchema.getDataType(), fieldSchema);
Review Comment:
TINYINT and SMALLINT partition keys come out wrong for negative values.
`PaimonSchemaExtractor` maps both to `InternalType.INT`, so `getValue` reads
them with `row.getInt`, but Paimon writes them as 1 and 2 bytes
(`writeByte`/`writeShort`). A TINYINT of -1 comes back as 255, and a SMALLINT
of -2 as 65534. Before this change the Delta target got them right, because the
string path read them with `getByte`/`getShort`.
Could `getValue` pick `getByte`/`getShort` from the Paimon type?
`InternalType` alone can't tell them apart, so it probably needs the Paimon
`DataType`, e.g. from `table.store().partitionType()` here. Column stats go
through the same read, so their min/max for negative TINYINT/SMALLINT values
are already off. A negative TINYINT case in the new unit test would cover it.
##########
xtable-core/src/test/java/org/apache/xtable/paimon/TestPaimonPartitionExtractor.java:
##########
@@ -123,6 +126,44 @@ void testToPartitionValuesWithPartitionedTable() {
assertEquals(Range.scalar("INFO"), partitionValue.getRange());
}
+ @Test
+ void testToPartitionValuesWithTypedPartitionKeys() {
+ Schema paimonSchema =
+ Schema.newBuilder()
+ .column("id", DataTypes.INT())
+ .column("dt", DataTypes.DATE())
+ .column("bucket_num", DataTypes.INT())
+ .column("event_id", DataTypes.BIGINT())
+ .primaryKey("id", "dt", "bucket_num", "event_id")
+ .partitionKeys("dt", "bucket_num", "event_id")
+ .option("bucket", "1")
+ .option("bucket-key", "id")
+ .build();
+ FileStoreTable paimonTable =
+ ((TestPaimonTable)
+ TestPaimonTable.createTable(
+ "typed_table", "dt", tempDir, new Configuration(), false,
paimonSchema))
+ .getPaimonTable();
+ InternalSchema schema =
+
PaimonSchemaExtractor.getInstance().toInternalSchema(paimonTable.schema());
+
+ int epochDay = (int) LocalDate.of(2019, 10, 12).toEpochDay();
+ BinaryRow partition = new BinaryRow(3);
+ BinaryRowWriter writer = new BinaryRowWriter(partition);
+ writer.writeInt(0, epochDay);
+ writer.writeInt(1, 7);
+ writer.writeLong(2, 42L);
+ writer.complete();
+
+ List<PartitionValue> result = extractor.toPartitionValues(paimonTable,
partition, schema);
+
+ assertEquals(3, result.size());
+ assertEquals("dt",
result.get(0).getPartitionField().getSourceField().getName());
+ assertEquals(Range.scalar(epochDay), result.get(0).getRange());
+ assertEquals(Range.scalar(7), result.get(1).getRange());
+ assertEquals(Range.scalar(42L), result.get(2).getRange());
Review Comment:
The description lists two behavior changes that aren't tested: a null
partition value now comes back as `null` instead of the default partition name,
and an unsupported partition type now throws `ReadException`. Could we add a
case for each? For the null case, `writer.setNullAt(...)` on one of the fields
would do it, and an `assertThrows` with a BYTES partition key would cover the
other.
##########
xtable-core/src/main/java/org/apache/xtable/paimon/PaimonPartitionExtractor.java:
##########
@@ -62,21 +62,26 @@ public List<InternalPartitionField>
toInternalPartitionFields(
public List<PartitionValue> toPartitionValues(
FileStoreTable table, BinaryRow partition, InternalSchema
internalSchema) {
- InternalRowPartitionComputer partitionComputer =
newPartitionComputer(table);
- Map<String, String> partValues =
partitionComputer.generatePartValues(partition);
-
- if (partValues.isEmpty()) {
- return Collections.emptyList();
- }
-
- List<PartitionValue> partitionValues = new ArrayList<>(partValues.size());
- for (Map.Entry<String, String> entry : partValues.entrySet()) {
- PartitionValue partitionValue =
+ List<String> partitionKeys = table.partitionKeys();
+ List<PartitionValue> partitionValues = new
ArrayList<>(partitionKeys.size());
+ for (int i = 0; i < partitionKeys.size(); i++) {
+ InternalPartitionField partitionField =
+ toPartitionField(partitionKeys.get(i), internalSchema);
+ InternalSchema fieldSchema = partitionField.getSourceField().getSchema();
+ // read the typed value from the partition row, the string form from
+ // InternalRowPartitionComputer is only meant for paths
+ Object value = statsExtractor.getValue(partition, i,
fieldSchema.getDataType(), fieldSchema);
+ if (value == null && !partition.isNullAt(i)) {
+ throw new ReadException(
Review Comment:
nit: for BYTES and the other types with no reader, `getValue` first logs a
WARN saying it is "skipping stats for this column", and then this throws. The
log is misleading on the partition path. Could the type check happen here
before the call, or could `getValue` stay quiet and let each caller decide what
to do?
--
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]
