albertobastos commented on code in PR #15692: URL: https://github.com/apache/pinot/pull/15692#discussion_r2076945503
########## pinot-common/src/main/java/org/apache/pinot/common/catalog/PinotNameMatcher.java: ########## @@ -0,0 +1,121 @@ +/** + * 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.common.catalog; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import javax.annotation.Nullable; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeField; +import org.apache.calcite.sql.validate.SqlNameMatcher; +import org.apache.pinot.common.utils.list.FlatViewList; + + +/** + * A custom name matcher that, although matches names based on config-provided case-sensitiveness, always reports to be + * case-sensitive so Calcite does not transform identifiers to lower case. + */ +public class PinotNameMatcher implements SqlNameMatcher { + + private final boolean _caseSensitive; + + public PinotNameMatcher(boolean caseSensitive) { + _caseSensitive = caseSensitive; + } + + @Override + public boolean isCaseSensitive() { + return true; + } + + @Override + public boolean matches(String string, String name) { + return _caseSensitive ? string.equals(name) : string.equalsIgnoreCase(name); + } + + @Nullable + @Override + public <K extends List<String>, V> V get(Map<K, V> map, List<String> prefixNames, List<String> names) { + if (_caseSensitive) { + List<String> key = concat(prefixNames, names); + return map.get(key); + } else { + for (Map.Entry<K, V> entry : map.entrySet()) { + if (listMatches(prefixNames, names, entry.getKey())) { + return entry.getValue(); + } + } + + return null; + } + } + + @Override + public String bestString() { + throw new UnsupportedOperationException(); + } + + @Override + public @Nullable RelDataTypeField field(RelDataType rowType, String fieldName) { Review Comment: Done. ########## pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java: ########## @@ -116,6 +117,13 @@ @Value.Enclosing public class QueryEnvironment { private static final Logger LOGGER = LoggerFactory.getLogger(QueryEnvironment.class); + private static final CalciteConnectionConfig CONNECTION_CONFIG; + + static { + Properties connectionConfigProperties = new Properties(); + connectionConfigProperties.setProperty(CalciteConnectionProperty.CASE_SENSITIVE.camelName(), "true"); Review Comment: Yeah, that's a good idea. Added a quite throughful java comment explaining why we don't want Calcite to enable its built-in case-insensitive behavior. ########## pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java: ########## @@ -68,10 +67,15 @@ public Table getTable(String name) { String rawTableName = TableNameBuilder.extractRawTableName(name); String physicalTableName = DatabaseUtils.translateTableName(rawTableName, _databaseName); String tableName = _tableCache.getActualTableName(physicalTableName); + if (tableName == null) { + return null; Review Comment: Done. ########## pinot-common/src/main/java/org/apache/pinot/common/catalog/PinotNameMatcher.java: ########## @@ -0,0 +1,121 @@ +/** + * 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.common.catalog; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import javax.annotation.Nullable; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeField; +import org.apache.calcite.sql.validate.SqlNameMatcher; +import org.apache.pinot.common.utils.list.FlatViewList; + + +/** + * A custom name matcher that, although matches names based on config-provided case-sensitiveness, always reports to be + * case-sensitive so Calcite does not transform identifiers to lower case. + */ +public class PinotNameMatcher implements SqlNameMatcher { + + private final boolean _caseSensitive; + + public PinotNameMatcher(boolean caseSensitive) { + _caseSensitive = caseSensitive; + } + + @Override + public boolean isCaseSensitive() { + return true; + } + + @Override + public boolean matches(String string, String name) { + return _caseSensitive ? string.equals(name) : string.equalsIgnoreCase(name); + } + + @Nullable + @Override + public <K extends List<String>, V> V get(Map<K, V> map, List<String> prefixNames, List<String> names) { + if (_caseSensitive) { + List<String> key = concat(prefixNames, names); + return map.get(key); + } else { + for (Map.Entry<K, V> entry : map.entrySet()) { + if (listMatches(prefixNames, names, entry.getKey())) { + return entry.getValue(); + } + } + + return null; + } + } + + @Override + public String bestString() { + throw new UnsupportedOperationException(); + } + + @Override + public @Nullable RelDataTypeField field(RelDataType rowType, String fieldName) { + return rowType.getField(fieldName, _caseSensitive, false); + } + + @Override + public int frequency(Iterable<String> names, String name) { + int n = 0; + + for (String s : names) { + if (this.matches(s, name)) { + ++n; + } + } + + return n; + } + + @Override + public Set<String> createSet() { + return (this.isCaseSensitive() ? new LinkedHashSet<>() : new TreeSet<>(String.CASE_INSENSITIVE_ORDER)); + } + + private static List<String> concat(List<String> prefixNames, List<String> names) { + return new FlatViewList<>(prefixNames, names); Review Comment: Is an adaptation from the original `listMatches(list0, list1)` in Calcite's code so it receives two input lists and avoids allocation of a third, concatenated array. Added a comment explaining it. ########## pinot-common/src/main/java/org/apache/pinot/common/catalog/PinotNameMatcher.java: ########## @@ -0,0 +1,121 @@ +/** + * 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.common.catalog; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import javax.annotation.Nullable; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeField; +import org.apache.calcite.sql.validate.SqlNameMatcher; +import org.apache.pinot.common.utils.list.FlatViewList; + + +/** + * A custom name matcher that, although matches names based on config-provided case-sensitiveness, always reports to be + * case-sensitive so Calcite does not transform identifiers to lower case. + */ Review Comment: Actually apart from the forced `isCaseSensitive() == true` we also made some performance improvements over Calcite's implementation to avoid unnecessary allocations but yeah, added as part of the class comment to know that it should be (almost) aligned with the `SqlNameMatchers.BaseMatcher` implementation. ########## pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java: ########## @@ -116,6 +117,13 @@ @Value.Enclosing public class QueryEnvironment { private static final Logger LOGGER = LoggerFactory.getLogger(QueryEnvironment.class); Review Comment: Right, removed. ########## pinot-common/src/main/java/org/apache/pinot/common/catalog/PinotCatalogReader.java: ########## @@ -0,0 +1,35 @@ +/** + * 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.common.catalog; + +import com.google.common.collect.ImmutableList; +import java.util.List; +import org.apache.calcite.config.CalciteConnectionConfig; +import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.prepare.CalciteCatalogReader; +import org.apache.calcite.rel.type.RelDataTypeFactory; + + +public class PinotCatalogReader extends CalciteCatalogReader { Review Comment: Added class comments. ########## pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java: ########## @@ -80,7 +81,7 @@ public class HelixInstanceDataManager implements InstanceDataManager { private static final Logger LOGGER = LoggerFactory.getLogger(HelixInstanceDataManager.class); - private final ConcurrentHashMap<String, TableDataManager> _tableDataManagerMap = new ConcurrentHashMap<>(); + private final Map<String, TableDataManager> _tableDataManagerMap = new ConcurrentHashMap<>(); Review Comment: Yeah that's a leftover from the previous attempt that changed this internal maps. However, isn't it recommended to use the most possible abstract definition to avoid coupling with the actual used implementation? ########## pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/PinotDispatchPlanner.java: ########## @@ -75,6 +76,19 @@ public DispatchableSubPlan createDispatchableSubPlan(SubPlan subPlan) { return finalizeDispatchableSubPlan(rootFragment, context); } + private Set<String> resolveActualTableNames(Set<String> tableNames) { Review Comment: I'm afraid it is, because under some circumstances `SubPlanMetadata` receives the table names directly from Calcite's `RelRoot` which still preserves the original table names provided by the user and not the actual, resolved ones. -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
