yashmayya commented on code in PR #17047:
URL: https://github.com/apache/pinot/pull/17047#discussion_r2446106753
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotTable.java:
##########
@@ -61,4 +76,8 @@ public boolean isRolledUp(String s) {
public Enumerable<Object[]> scan(DataContext dataContext) {
return null;
}
+
+ private boolean shouldExcludeVirtualColumns() {
Review Comment:
This method doesn't seem necessary?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##########
@@ -61,6 +62,23 @@ public RelDataType createRelDataTypeFromSchema(Schema
schema) {
return builder.build();
}
+ /**
+ * Creates RelDataType from Schema excluding virtual columns (columns
starting with '$').
+ * This is used for NATURAL JOIN operations where virtual columns should not
participate
+ * in join condition matching.
+ */
+ public RelDataType createRelDataTypeFromSchemaExcludingVirtualColumns(Schema
schema) {
Review Comment:
This is almost identical to `createRelDataTypeFromSchema` - I'd suggest
refactoring it to a method that takes in a `Predicate<String>` to check whether
a given column should be excluded. The regular method can just pass in `column
-> false` since no column will be excluded and the natural join case can use
the method reference `Validator::isVirtualColumn`.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -174,12 +177,65 @@ public QueryEnvironment(String database, TableCache
tableCache, @Nullable Worker
.build());
}
+ /**
+ * Configures virtual column exclusion for catalog tables based on query
analysis.
+ * Refer: https://github.com/apache/pinot/issues/15522
+ * This method detects NATURAL JOIN operations and sets the appropriate flag
on PinotTable instances.
+ */
+ private void configureVirtualColumnExclusion(SqlNodeAndOptions
sqlNodeAndOptions) {
+ boolean excludeVirtualColumns = false;
+ if (containsNaturalJoin(sqlNodeAndOptions.getSqlNode())) {
+ excludeVirtualColumns = true;
+ }
+
+ if (excludeVirtualColumns) {
+ _catalog.configureVirtualColumnExclusion(true);
+ }
Review Comment:
nit: can be simplified:
```
if (containsNaturalJoin(sqlNodeAndOptions.getSqlNode())) {
_catalog.configureVirtualColumnExclusion(true);
}
```
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -82,7 +92,9 @@ public Table getTable(String name) {
return null;
}
- return new PinotTable(schema);
+ PinotTable table = new PinotTable(schema);
+ table.setExcludeVirtualColumns(_excludeVirtualColumns);
Review Comment:
This feels awkward - why not create a new constructor instead?
--
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]
