yashmayya commented on PR #19200: URL: https://github.com/apache/pinot/pull/19200#issuecomment-5246968450
Thanks for picking this up. The root cause in the description is correct. Calcite reads `dim_tbl.currency = 'gbp'` as a non-equi condition, so the lookup key is shorter than the dimension table primary key. I looked at the change closely and found some problems. I list them here for the record. **1. A constant replaces an equi-join key.** The literal pass runs after the equi-join key pass, and it overwrites without a condition: ```java _keyColumnLeftIds[pkPos] = -1; // drops the equi-join key ``` Take `ON dim.currency = fact.currency AND dim.rate_start_date = fact.rate_start_date AND dim.currency = 'gbp'`. The first position of the key becomes the constant `gbp` for every fact row. A fact row with `usd` then reads the `gbp` dimension row. The equi-join key is not in the non-equi conditions, so no filter removes that row. The result is wrong rows, not missing rows. I reproduced this order of the two passes in a test and got 4 rows where 2 are correct. **2. The literal keeps the type that the planner gave it.** `PrimaryKey` compares values with `equals`, where an `Integer` never equals a `Long`. A literal of the wrong numeric width misses every row. The value needs a conversion to the stored type of the dimension column. **3. An open primary key column gives no error.** When no condition fills a position, the key holds a null there and the query returns 0 rows in silence. **4. A join key on a column outside the primary key is now dropped.** The equi-join key pass skips a column that is absent from the primary key. That condition is also absent from the non-equi conditions, so no filter applies it. The old code returned 0 rows for this query, and the change returns rows that do not match the condition. The single-stage `lookup` transform function rejects this case, and the multi-stage lookup join can do the same. **5. The change breaks every existing lookup join test.** `ResourceBasedQueriesTest.registerMockDimensionTable` mocks `DimensionTableDataManager` and does not stub `getPrimaryKeyColumns()`. Mockito returns an empty list, so `_keyColumnCount` is 0 and every lookup key is empty. This is why "Pinot Unit Test Set 1" fails on this branch. **6. The new test cannot pass, even after a correct fix.** `QueryRunnerTestBase.toRow` puts the raw JSON values into the row with no conversion to the type of the column. The fact side reads real segments, so it gets the type of the column. The test declares `rate_start_date` as `LONG`, so the mock map holds an `Integer` while the query supplies a `Long`. The mock needs the same conversion as a real dimension table. **7. Two smaller points.** The file loses its last newline, which the linter reports. There is also no test for the order fault. The same two conditions in reverse primary key order give 0 rows, because the key positions follow the join condition instead of the primary key. I opened #19210, which covers these cases and corrects the two faults in the test harness. It supersedes this pull request. -- 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]
