ankitsultana commented on code in PR #10423:
URL: https://github.com/apache/pinot/pull/10423#discussion_r1152422958
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -130,9 +131,16 @@ protected Object[][] provideQueries() {
public static QueryEnvironment getQueryEnvironment(int reducerPort, int
port1, int port2,
Map<String, Schema> schemaMap, Map<String, List<String>> segmentMap1,
Map<String, List<String>> segmentMap2) {
+ return getQueryEnvironment(reducerPort, port1, port2, schemaMap,
segmentMap1, segmentMap2, Collections.emptyMap());
+ }
+
+ public static QueryEnvironment getQueryEnvironment(int reducerPort, int
port1, int port2,
+ Map<String, Schema> schemaMap, Map<String, List<String>> segmentMap1,
Map<String, List<String>> segmentMap2,
+ Map<String, Boolean> nullHandlingMap) {
MockRoutingManagerFactory factory = new MockRoutingManagerFactory(port1,
port2);
for (Map.Entry<String, Schema> entry : schemaMap.entrySet()) {
- factory.registerTable(entry.getValue(), entry.getKey());
+ // ALWAYS enable null handling in V2.
+ factory.registerTable(entry.getValue(), entry.getKey(),
nullHandlingMap.getOrDefault(entry.getKey(), false));
Review Comment:
Does this need to be: `nullHandlingMap.getOrDefault(entry.getKey(), true)`?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -43,43 +43,44 @@ public TypeFactory(RelDataTypeSystem typeSystem) {
super(typeSystem);
}
- public RelDataType createRelDataTypeFromSchema(Schema schema) {
+ public RelDataType createRelDataTypeFromSchema(Schema schema, boolean
isNullSupportEnabled) {
Builder builder = new Builder(this);
for (Map.Entry<String, FieldSpec> e : schema.getFieldSpecMap().entrySet())
{
- builder.add(e.getKey(), toRelDataType(e.getValue()));
+ builder.add(e.getKey(), toRelDataType(e.getValue(),
+ isNullSupportEnabled || e.getValue().isNullableField()));
Review Comment:
Should this be "and"? `isNullSupportEnabled &&
e.getValue().isNullableField()`
Since `fieldSpec` is already passed to `toRelDataType`, we can also pass
only `isNullSupportEnabled` here and in `toRelDataType` we can do:
```
return createTypeWithNullability(fieldSpec.isSingleValueField() ?
createSqlType(SqlTypeName.INTEGER)
: createArrayType(createSqlType(SqlTypeName.INTEGER), -1),
isNullSupportEnabled && fieldSpec.isNullableField());
```
##########
pinot-query-runtime/src/test/resources/queries/NullHandling.json:
##########
@@ -0,0 +1,149 @@
+{
+ "tbl_level_null_handling": {
Review Comment:
Can you also add some plan-based tests in pinot-query-planner?
Particularly for anti semi-join queries such as: `SELECT .. FROM T1 WHERE
uuid NOT IN (SELECT uuid FROM ...)`
If the uuid is column is nullable, this gets converted to a broadcast join
(iirc).
Would be great if we can add test-cases for these.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -127,4 +128,20 @@ public boolean isMutable() {
public Schema snapshot(SchemaVersion version) {
return this;
}
+
+ private static boolean isNullEnabled(TableCache tableCache, String
rawTableName) {
+ TableConfig tableConfig = tableCache.getTableConfig(
Review Comment:
We need to get TableConfig for at least one type in a bunch of places in the
code. Can we add a util method somewhere?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotTable.java:
##########
@@ -37,16 +37,18 @@
*/
public class PinotTable extends AbstractTable implements ScannableTable {
private Schema _schema;
+ private boolean _enableNullSupport;
Review Comment:
nit: both Schema and enableNullSupport can be final
--
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]
