Jackie-Jiang commented on code in PR #15773:
URL: https://github.com/apache/pinot/pull/15773#discussion_r2098665701
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/InstanceDataManager.java:
##########
@@ -201,4 +201,10 @@ void reloadSegments(String tableNameWithType, List<String>
segmentNames, boolean
* Returns the instance data directory
*/
String getInstanceDataDir();
+
+ /**
+ * Returns the logical table config and schema for the given logical table
name.
+ */
+ @Nullable
Review Comment:
Consider directly throw exception when some config is not available.
Please also add a TODO to remove this method because we should never read ZK
at query path.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/LogicalTableManager.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * 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.core.data.manager;
+
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.LogicalTableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+
+public class LogicalTableManager {
Review Comment:
This is not really a manager. You may call it LogicalTableConfigs
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/LogicalTableManager.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * 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.core.data.manager;
+
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.LogicalTableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+
+public class LogicalTableManager {
+ private final LogicalTableConfig _logicalTableConfig;
+ private final Schema _logicalTableSchema;
+ private final TableConfig _refOfflineTableConfig;
+ private final TableConfig _refRealtimeTableConfig;
+
+ public LogicalTableManager(LogicalTableConfig logicalTableConfig, Schema
logicalTableSchema,
+ TableConfig refOfflineTableConfig, TableConfig refRealtimeTableConfig) {
+ _logicalTableConfig = logicalTableConfig;
+ _logicalTableSchema = logicalTableSchema;
+ _refOfflineTableConfig = refOfflineTableConfig;
+ _refRealtimeTableConfig = refRealtimeTableConfig;
+ }
+
+ public LogicalTableConfig getLogicalTableConfig() {
+ return _logicalTableConfig;
+ }
+ public Schema getLogicalTableSchema() {
Review Comment:
(minor) Add some whitespace between methods
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -386,4 +410,72 @@ private static List<Expression>
computeInOperands(List<Object[]> dataContainer,
}
return expressions;
}
+
+ private static List<InstanceRequest>
constructLogicalTableServerQueryRequests(
+ OpChainExecutionContext executionContext, PinotQuery pinotQuery,
InstanceDataManager instanceDataManager) {
+ StageMetadata stageMetadata = executionContext.getStageMetadata();
+ String logicalTableName =
TableNameBuilder.extractRawTableName(stageMetadata.getTableName());
Review Comment:
Can logical table name ever have type suffix?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/DispatchablePlanContext.java:
##########
@@ -103,6 +103,8 @@ public Map<Integer, DispatchablePlanFragment>
constructDispatchablePlanFragmentM
dispatchablePlanMetadata.getWorkerIdToServerInstanceMap();
Map<Integer, Map<String, List<String>>> workerIdToSegmentsMap =
dispatchablePlanMetadata.getWorkerIdToSegmentsMap();
+ Map<Integer, DispatchablePlanMetadata.TableTypeTableNameToSegmentsMap>
workerIdToTableNameSegmentsMap =
Review Comment:
Consider adding a pre-check that this one cannot co-exist with
`workerIdToSegmentsMap`
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/DispatchablePlanMetadata.java:
##########
@@ -80,6 +81,24 @@ public class DispatchablePlanMetadata implements
Serializable {
// Map from workerId -> {planFragmentId -> mailboxes}
private final Map<Integer, Map<Integer, MailboxInfos>>
_workerIdToMailboxesMap = new HashMap<>();
+ /**
+ * Map from workerId -> {tableType -> {tableName -> segments}} is required
for logical tables.
+ * Raw definition of the map is:
+ * Map<String, Map<String, Map<String, List<String>>>>. Since this
definition is hard to understand - specifically
+ * what do each of the string keys store, we define two classes:
+ * {@link TableTypeToSegmentsMap} and {@link
TableTypeTableNameToSegmentsMap} to help read code more easily.
+ */
+ public static class TableTypeToSegmentsMap {
Review Comment:
I'd suggest just keeping the plain map. Each class introduced is adding
overhead on heap. Putting some javadoc explaining what each level stands for
should be good enough.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/DispatchablePlanMetadata.java:
##########
@@ -80,6 +81,24 @@ public class DispatchablePlanMetadata implements
Serializable {
// Map from workerId -> {planFragmentId -> mailboxes}
private final Map<Integer, Map<Integer, MailboxInfos>>
_workerIdToMailboxesMap = new HashMap<>();
+ /**
+ * Map from workerId -> {tableType -> {tableName -> segments}} is required
for logical tables.
+ * Raw definition of the map is:
+ * Map<String, Map<String, Map<String, List<String>>>>. Since this
definition is hard to understand - specifically
+ * what do each of the string keys store, we define two classes:
+ * {@link TableTypeToSegmentsMap} and {@link
TableTypeTableNameToSegmentsMap} to help read code more easily.
+ */
+ public static class TableTypeToSegmentsMap {
Review Comment:
Some of the map can be optimized into Pairs to reduce overhead
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/LogicalTableRouteInfo.java:
##########
@@ -32,22 +32,23 @@
import org.apache.pinot.core.routing.ServerRouteInfo;
import org.apache.pinot.core.routing.TimeBoundaryInfo;
import org.apache.pinot.core.transport.BaseTableRouteInfo;
+import org.apache.pinot.core.transport.ImplicitHybridTableRouteInfo;
import org.apache.pinot.core.transport.ServerInstance;
import org.apache.pinot.core.transport.ServerRoutingInstance;
import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.query.timeboundary.TimeBoundaryStrategy;
import org.apache.pinot.spi.config.table.QueryConfig;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.config.table.TableType;
-import org.apache.pinot.spi.data.LogicalTableConfig;
import org.apache.pinot.spi.query.QueryThreadContext;
import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
public class LogicalTableRouteInfo extends BaseTableRouteInfo {
- private final LogicalTableConfig _logicalTable;
- private List<TableRouteInfo> _offlineTables;
- private List<TableRouteInfo> _realtimeTables;
+ private String _logicalTableName;
+ private List<ImplicitHybridTableRouteInfo> _offlineTables;
Review Comment:
Why each physical table is wrapped as an implicit hybrid table? We should
have an abstraction for single physical table
--
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]
