xiangfu0 commented on code in PR #18873:
URL: https://github.com/apache/pinot/pull/18873#discussion_r3743216304
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryGroupKeyGeneratorTest.java:
##########
@@ -75,20 +77,31 @@ public class NoDictionaryGroupKeyGeneratorTest {
private static final String STRING_COLUMN = "stringColumn";
private static final String BYTES_COLUMN = "bytesColumn";
private static final String BYTES_DICT_COLUMN = "bytesDictColumn";
+ private static final String UUID_COLUMN = "uuidColumn";
+ private static final String BOOLEAN_COLUMN = "booleanColumn";
+ private static final String TIMESTAMP_COLUMN = "timestampColumn";
+ private static final String UUID_DICT_COLUMN = "uuidDictColumn";
private static final List<String> COLUMNS =
Arrays.asList(INT_COLUMN, LONG_COLUMN, FLOAT_COLUMN, DOUBLE_COLUMN,
STRING_COLUMN, BYTES_COLUMN,
- BYTES_DICT_COLUMN);
+ BYTES_DICT_COLUMN, UUID_COLUMN, BOOLEAN_COLUMN, TIMESTAMP_COLUMN,
UUID_DICT_COLUMN);
private static final int NUM_COLUMNS = COLUMNS.size();
+ private static final Set<String> UUID_COLUMNS = Set.of(UUID_COLUMN,
UUID_DICT_COLUMN);
private static final TableConfig TABLE_CONFIG = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME)
- .setNoDictionaryColumns(COLUMNS.subList(0, NUM_COLUMNS - 1)).build();
+ .setNoDictionaryColumns(
+ List.of(INT_COLUMN, LONG_COLUMN, FLOAT_COLUMN, DOUBLE_COLUMN,
STRING_COLUMN, BYTES_COLUMN, UUID_COLUMN,
+ BOOLEAN_COLUMN, TIMESTAMP_COLUMN)).build();
Review Comment:
Correction to my previous reply — I pasted the wrong measurement into this
thread. The correct figure for this line
(`NoDictionaryGroupKeyGeneratorTest:92`) is **57 characters**, not 59. The
conclusion is unchanged: it is well under the 120 limit, no line in the file
exceeds 120, and `checkstyle:check` passes on `pinot-core`.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/UuidAggregationTest.java:
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.pinot.integration.tests.custom;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.avro.file.DataFileWriter;
+import org.apache.avro.generic.GenericData;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+
+/// End-to-end coverage for aggregating, grouping and de-duplicating a UUID
column.
+///
+/// These run through a real broker reduce, which is the point: the group-key
conversion in
+/// `GroupByDataTableReducer#getConvertedKey` is only reachable when the
broker reduces a *single* data table, and the
+/// unit-level `BaseQueriesTest` harness always reduces two. A `case UUID`
there that returns the stored `byte[]`
+/// instead of the converted `java.util.UUID` makes `GROUP BY ... HAVING` over
a UUID column fail with
+/// `ClassCastException: class [B cannot be cast to class java.util.UUID`, and
only a query-level test catches it.
+@Test(suiteName = "CustomClusterIntegrationTest")
+public class UuidAggregationTest extends CustomDataQueryClusterIntegrationTest
{
+ private static final String TABLE_NAME = "UuidAggregationTest";
+ private static final String UUID_COLUMN = "uuidColumn";
+ private static final String UUID_0 = "550e8400-e29b-41d4-a716-446655440000";
+ private static final String UUID_0_HEX = "550e8400e29b41d4a716446655440000";
+ private static final String UUID_1 = "550e8400-e29b-41d4-a716-446655440001";
+ private static final String UUID_2 = "550e8400-e29b-41d4-a716-446655440002";
+
+ /// `UUID_0` appears twice so grouping and distinct are distinguishable from
a plain row count.
+ private static final List<String> ROWS = List.of(UUID_0, UUID_0, UUID_1,
UUID_2);
+ private static final int NUM_DISTINCT = 3;
+
+ @Override
+ public String getTableName() {
+ return TABLE_NAME;
+ }
+
+ @Override
+ protected long getCountStarResult() {
+ return ROWS.size();
+ }
+
+ @Override
+ public int getNumAvroFiles() {
+ return 1;
+ }
+
+ @Override
+ public TableConfig createOfflineTableConfig() {
+ return new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName()).build();
+ }
+
+ @Override
+ public Schema createSchema() {
+ return new Schema.SchemaBuilder().setSchemaName(getTableName())
+ .addSingleValueDimension(UUID_COLUMN, DataType.UUID)
+ .build();
+ }
+
+ @Override
+ public List<File> createAvroFiles()
+ throws Exception {
+ org.apache.avro.Schema avroSchema =
org.apache.avro.Schema.createRecord("uuidRecord", null, null, false);
+ avroSchema.setFields(List.of(new org.apache.avro.Schema.Field(UUID_COLUMN,
+ org.apache.avro.Schema.create(org.apache.avro.Schema.Type.STRING),
null, null)));
+
+ try (AvroFilesAndWriters avroFilesAndWriters =
createAvroFilesAndWriters(avroSchema)) {
+ DataFileWriter<GenericData.Record> writer =
avroFilesAndWriters.getWriters().get(0);
+ for (String uuid : ROWS) {
+ GenericData.Record record = new GenericData.Record(avroSchema);
+ record.put(UUID_COLUMN, uuid);
+ writer.append(record);
+ }
+ return avroFilesAndWriters.getAvroFiles();
+ }
+ }
+
+ @Test
+ public void testGroupByUuidColumn()
+ throws Exception {
+ setUseMultiStageQueryEngine(false);
+ JsonNode rows = query(
+ String.format("SELECT %s, COUNT(*) FROM %s GROUP BY %s ORDER BY %s",
UUID_COLUMN, getTableName(), UUID_COLUMN,
+ UUID_COLUMN));
+ assertEquals(rows.size(), NUM_DISTINCT, rows.toPrettyString());
+
+ // Group keys must come back as canonical UUIDs, not hex and not a
byte-array rendering.
+ List<String> keys = new ArrayList<>();
+ for (JsonNode row : rows) {
+ keys.add(row.get(0).asText());
+ }
+ assertEquals(keys, List.of(UUID_0, UUID_1, UUID_2), rows.toPrettyString());
+ assertEquals(rows.get(0).get(1).asLong(), 2, rows.toPrettyString());
+ assertEquals(rows.get(1).get(1).asLong(), 1, rows.toPrettyString());
+ }
+
+ /// The regression that motivated this class: `GROUP BY` a UUID column with
a `HAVING` predicate on that same
+ /// column runs the group key through `getConvertedKey` and then straight
into `PredicateRowMatcher`, which casts
+ /// to `java.util.UUID`.
+ @Test
+ public void testGroupByUuidColumnWithHaving()
+ throws Exception {
+ setUseMultiStageQueryEngine(false);
+ JsonNode rows = query(
+ String.format("SELECT %s, COUNT(*) FROM %s GROUP BY %s HAVING %s =
'%s'", UUID_COLUMN, getTableName(),
+ UUID_COLUMN, UUID_COLUMN, UUID_0_HEX));
+ assertEquals(rows.size(), 1, rows.toPrettyString());
+ assertEquals(rows.get(0).get(0).asText(), UUID_0, rows.toPrettyString());
+ assertEquals(rows.get(0).get(1).asLong(), 2, rows.toPrettyString());
+
+ // Same thing via an explicit CAST of the canonical form.
+ rows = query(String.format("SELECT %s, COUNT(*) FROM %s GROUP BY %s HAVING
%s = CAST('%s' AS UUID)", UUID_COLUMN,
+ getTableName(), UUID_COLUMN, UUID_COLUMN, UUID_1));
Review Comment:
Correction to my previous reply — I pasted a measurement for the wrong file.
This thread is `UuidAggregationTest:138`, which is **59 characters**; the
57-character figure I quoted belongs to `NoDictionaryGroupKeyGeneratorTest:92`.
The conclusion is unchanged: under the 120 limit, no line in this file exceeds
120, and `checkstyle:check` passes.
--
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]
