Copilot commented on code in PR #19210:
URL: https://github.com/apache/pinot/pull/19210#discussion_r3770889117
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java:
##########
@@ -301,13 +311,39 @@ private void registerMockDimensionTable(String
offlineTableName, Schema schema,
}
Object[] values = new Object[columns.length];
for (int i = 0; i < columns.length; i++) {
- values[i] = row.getValue(columns[i]);
+ values[i] = toStoredValue(row.getValue(columns[i]), schema,
columns[i]);
}
return values;
});
DimensionTableDataManager.registerDimensionTable(offlineTableName,
mockDimManager);
}
+ /// Converts a raw JSON value to the stored representation of its column,
the same way a segment stores it.
+ @Nullable
+ private static Object toStoredValue(@Nullable Object value, Schema schema,
String column) {
+ if (value == null) {
+ return null;
+ }
+ FieldSpec fieldSpec = schema.getFieldSpecFor(column);
+ if (fieldSpec == null) {
+ return value;
+ }
+ switch (fieldSpec.getDataType().getStoredType()) {
+ case INT:
+ return ((Number) value).intValue();
+ case LONG:
+ return ((Number) value).longValue();
+ case FLOAT:
+ return ((Number) value).floatValue();
+ case DOUBLE:
+ return ((Number) value).doubleValue();
+ case STRING:
+ return value.toString();
+ default:
+ return value;
+ }
Review Comment:
This conversion is incomplete for supported scalar types. A BOOLEAN has
stored type INT but arrives from JSON as a Boolean, so the Number cast throws;
TIMESTAMP may need parsing, while BIG_DECIMAL and BYTES currently fall through
with representations that differ from a real segment. That makes the mock
diverge from `PinotSegmentRecordReader` and can produce false failures or
misses. Reuse the existing `FieldSpec.DataType` conversion instead of
maintaining a partial stored-type switch.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LookupJoinOperator.java:
##########
@@ -97,10 +122,179 @@ public LookupJoinOperator(OpChainExecutionContext
context, MultiStageOperator le
_resultSchema = node.getDataSchema();
_resultColumnSize = _resultSchema.size();
List<RexExpression> nonEquiConditions = node.getNonEquiConditions();
+ // SEMI and ANTI joins project the left columns only, so an evaluator
built over the join result schema cannot
+ // reference a dimension table column. Reject the combination here,
otherwise the loop below fails with an index
+ // error that says nothing about the cause.
+ Preconditions.checkState(nonEquiConditions.isEmpty() ||
_joinType.projectsRight(),
+ "Lookup join type: %s does not support non-equi join conditions, got:
%s", _joinType, nonEquiConditions);
Review Comment:
The new SEMI/ANTI rejection path is not covered by the added tests, although
it is a user-visible behavior change intended to replace an internal index
error with this diagnostic. Add focused SEMI and ANTI lookup-join cases with a
non-equi condition and assert this message, so both join-type branches remain
protected.
##########
pinot-query-runtime/src/test/resources/queries/LookupJoin.json:
##########
@@ -75,5 +75,172 @@
"ignoreLiteMode": true
}
]
+ },
+ "lookup_join_literal_key": {
+ "comment": "Regression test for issue 19188. One dimension primary-key
component comes from a literal in the join condition. Calcite classifies that
condition as a non-equi condition, so it is absent from the join keys. Before
the fix the lookup key held one value against a two-column primary key and the
join returned 0 rows.",
+ "tables": {
+ "fact_tbl": {
+ "schema": [
+ {"name": "rate_start_date", "type": "LONG"}
+ ],
+ "inputs": [
+ [1]
+ ]
+ },
+ "dim_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "rate", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 125],
+ ["usd", 1, 100]
+ ],
+ "replicated": true,
+ "isDimTable": true,
+ "primaryKeyColumns": ["currency", "rate_start_date"]
+ }
+ },
+ "queries": [
+ {
+ "description": "Lookup join with a literal dimension primary-key
component",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON
{dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date =
{fact_tbl}.rate_start_date",
+ "outputs": [
+ ["gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ }
+ ]
+ },
+ "lookup_join_composite_key": {
+ "comment": "Covers lookup joins against a two-column dimension primary
key. The lookup key must hold one value per primary key column, in the order
the dimension table schema declares them. These cases check that the key is
complete, that it is ordered by the primary key instead of by the join
condition, and that a join condition which cannot produce a complete key gives
an error instead of 0 rows.",
+ "tables": {
+ "fact_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "amount", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 10],
+ ["usd", 1, 20],
+ ["gbp", 2, 30],
+ ["eur", 1, 40],
+ ["eur", 9, 50]
+ ]
+ },
+ "dim_tbl": {
+ "schema": [
+ {"name": "currency", "type": "STRING"},
+ {"name": "rate_start_date", "type": "LONG"},
+ {"name": "rate", "type": "INT"}
+ ],
+ "inputs": [
+ ["gbp", 1, 125],
+ ["usd", 1, 100],
+ ["gbp", 2, 130]
+ ],
+ "replicated": true,
+ "isDimTable": true,
+ "primaryKeyColumns": ["currency", "rate_start_date"]
+ }
+ },
+ "queries": [
+ {
+ "description": "Literal primary-key component in the ON clause",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN
{dim_tbl} ON {dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date =
{fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, "gbp", 125],
+ [20, "gbp", 125],
+ [30, "gbp", 130],
+ [40, "gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Same literal primary-key component, written in the
WHERE clause. The planner moves it into the join condition, so the operator
sees the same plan.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.currency, {dim_tbl}.rate FROM {fact_tbl} JOIN
{dim_tbl} ON {dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date WHERE
{dim_tbl}.currency = 'gbp'",
+ "outputs": [
+ [10, "gbp", 125],
+ [20, "gbp", 125],
+ [30, "gbp", 130],
+ [40, "gbp", 125]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Left join with a literal primary-key component. Before
the fix every row was null-padded, which gave wrong values instead of missing
rows.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} LEFT JOIN {dim_tbl} ON
{dim_tbl}.currency = 'gbp' AND {dim_tbl}.rate_start_date =
{fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, 125],
+ [20, 125],
+ [30, 130],
+ [40, 125],
+ [50, null]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Both primary-key columns equi-joined, conditions
written in primary key order",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON
{dim_tbl}.currency = {fact_tbl}.currency AND {dim_tbl}.rate_start_date =
{fact_tbl}.rate_start_date",
+ "outputs": [
+ [10, 125],
+ [20, 100],
+ [30, 130]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "Both primary-key columns equi-joined, conditions
written in reverse primary key order. Before the fix the key values were
swapped and the join returned 0 rows.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON
{dim_tbl}.rate_start_date = {fact_tbl}.rate_start_date AND {dim_tbl}.currency =
{fact_tbl}.currency",
+ "outputs": [
+ [10, 125],
+ [20, 100],
+ [30, 130]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "A literal on a primary-key column that an equi-join
key already binds. The equi-join key builds the key and the literal runs as a
filter after the lookup. If the literal replaced the equi-join key, the usd
fact row would read the gbp dimension row and add a wrong row.",
+ "sql": "SELECT /*+ joinOptions(join_strategy='lookup') */
{fact_tbl}.amount, {dim_tbl}.rate FROM {fact_tbl} JOIN {dim_tbl} ON
{dim_tbl}.currency = {fact_tbl}.currency AND {dim_tbl}.rate_start_date =
{fact_tbl}.rate_start_date AND {dim_tbl}.currency = 'gbp'",
+ "outputs": [
+ [10, 125],
+ [30, 130]
+ ],
+ "ignoreLiteMode": true
+ },
+ {
+ "description": "A literal binds the LONG primary-key column. The
planner types the literal, and a literal of the wrong numeric width misses
every row, so the operator converts it to the stored type of the column.",
Review Comment:
This description contradicts the implementation and PR contract: the
operator deliberately does not convert constants; it only verifies the
planner-provided stored type. Please describe that validation so this
regression test does not document behavior that does not exist.
--
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]
