wombatu-kun commented on code in PR #19483:
URL: https://github.com/apache/hudi/pull/19483#discussion_r3704296649
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/util/HudiAvroSerializer.java:
##########
@@ -170,21 +177,36 @@ public Object getValue(Page sourcePage, int channel, int
position)
public void buildRecordInPage(PageBuilder pageBuilder, IndexedRecord
record)
{
pageBuilder.declarePosition();
- int blockSeq = 0;
- for (int channel = 0; channel < columnTypes.size(); channel++,
blockSeq++) {
- BlockBuilder output = pageBuilder.getBlockBuilder(blockSeq);
- HiveColumnHandle columnHandle = columnHandles.get(channel);
- if (prefilledColumnValues.isPrefilled(columnHandle)) {
- prefilledColumnValues.appendTo(columnHandle, output);
+ // Record may not be projected, get field positions from its own schema
+ int[] fieldPositions = fieldPositionsFor(record.getSchema());
+ for (int channel = 0; channel < columnTypes.size(); channel++) {
+ BlockBuilder output = pageBuilder.getBlockBuilder(channel);
+ int fieldPosition = fieldPositions[channel];
+ if (fieldPosition < 0) {
+ prefilledColumnValues.appendTo(columnHandles.get(channel),
output);
Review Comment:
`PrefilledColumnValues.appendTo` re-parses the split-constant partition
value through `HiveUtil.getPrefilledColumnValue` on every record, which now
costs more per row than the field lookup this PR caches. Worth memoizing the
resolved value per column in `PrefilledColumnValues` -
`HudiBaseFileOnlyPageSource` already gets it once per page via `toRleBlock` -
or is that out of scope here?
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/util/HudiAvroSerializer.java:
##########
@@ -524,10 +548,17 @@ private static void writeMap(MapBlockBuilder output,
MapType mapType, Map<?, ?>
static class AvroDecimalConverter
{
private static final Conversions.DecimalConversion
AVRO_DECIMAL_CONVERSION = new Conversions.DecimalConversion();
+ // convert() runs once per decimal cell on the record read path, and
building a Schema costs
+ // orders of magnitude more than the conversion itself. The
(precision, scale) space is tiny
+ // and fixed per column, so cache the schemas globally.
+ private static final Map<Integer, Schema> DECIMAL_SCHEMAS = new
ConcurrentHashMap<>();
Review Comment:
`AvroDecimalConverter`'s only caller is the short-decimal branch, where
`fromBytes` plus `encodeShortScaledValue` reduce to `new
BigInteger(fixed.bytes()).longValueExact()` - Avro's `DecimalConversion` reads
only the scale, so precision never reaches the result. Is the schema cache
worth keeping over dropping the converter and the map entirely?
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/util/TestHudiAvroSerializer.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.hudi.util;
+
+import io.trino.metastore.HiveType;
+import io.trino.plugin.hive.HiveColumnHandle;
+import io.trino.plugin.hive.HivePartitionKey;
+import io.trino.plugin.hudi.HudiSplit;
+import io.trino.plugin.hudi.file.HudiBaseFile;
+import io.trino.spi.Page;
+import io.trino.spi.PageBuilder;
+import io.trino.spi.SplitWeight;
+import io.trino.spi.block.Block;
+import io.trino.spi.block.BlockBuilder;
+import io.trino.spi.predicate.TupleDomain;
+import io.trino.spi.type.DecimalType;
+import org.apache.avro.Schema;
+import org.apache.avro.SchemaBuilder;
+import org.apache.avro.generic.GenericData;
+import org.junit.jupiter.api.Test;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.Optional;
+
+import static io.trino.spi.type.BigintType.BIGINT;
+import static io.trino.spi.type.IntegerType.INTEGER;
+import static io.trino.spi.type.VarcharType.VARCHAR;
+import static org.assertj.core.api.Assertions.assertThat;
+
+class TestHudiAvroSerializer
+{
+ @Test
+ public void testDecimalConverter()
+ {
+ HudiAvroSerializer.AvroDecimalConverter converter = new
HudiAvroSerializer.AvroDecimalConverter();
+
+ assertThat(converter.convert(10, 2,
unscaledBytes("123.45"))).isEqualTo(new BigDecimal("123.45"));
+ // Same (precision, scale) again: served from the cached schema
+ assertThat(converter.convert(10, 2,
unscaledBytes("-0.07"))).isEqualTo(new BigDecimal("-0.07"));
+ // Same precision, different scale, and vice versa: must not collide
in the cache
+ assertThat(converter.convert(10, 4,
unscaledBytes("123.4567"))).isEqualTo(new BigDecimal("123.4567"));
+ assertThat(converter.convert(18, 2,
unscaledBytes("9999999999999999.99"))).isEqualTo(new
BigDecimal("9999999999999999.99"));
Review Comment:
The same-scale/different-precision case cannot fail for any key that
includes the scale, since Avro's `DecimalConversion` derives the value from the
scale alone. Either assert the cached schema's precision directly or drop that
half of the comment.
--
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]
