Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang merged PR #15388: URL: https://github.com/apache/pinot/pull/15388 -- 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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2076425223
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+ private Map _offlineRoutingTable;
+ private Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo() {
Review Comment:
I see. We can revisit later
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2074744804
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+ private Map _offlineRoutingTable;
+ private Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo() {
+ }
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
Review Comment:
I'll do it here. Should the arguments for setters also be set to annotated ?
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2074743868
##
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java:
##
@@ -92,6 +88,16 @@ public AsyncQueryResponse submitQuery(long requestId, String
rawTableName,
@Nullable Map offlineRoutingTable,
@Nullable BrokerRequest realtimeBrokerRequest,
@Nullable Map realtimeRoutingTable,
long timeoutMs) {
+TableRouteInfo tableRouteInfo = new
ImplicitHybridTableRouteInfo(offlineBrokerRequest, realtimeBrokerRequest,
Review Comment:
Also in `PinotServerDataFetcher.scala`. You dont see a diff because I kept
this function to avoid changes in that file.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2074743454
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+ private Map _offlineRoutingTable;
+ private Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo() {
Review Comment:
I used setter/getters because this class is filled up in two phases as we've
discussed on Friday. Also only broker request & routing table is required in
`PinotServerDataFetcher.scala`. Its not possible to use final & getters only
because of that. Other options are:
* Make the members set in getTableRoute final such as table names.
* Use a builder.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2074741444
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+ private Map _offlineRoutingTable;
+ private Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo() {
+ }
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
Review Comment:
This constructor is used in `QueryRoutingTest`. It makes the diff smaller.
More importantly this function is used `PinotServerDataFetcher.scala`. If this
function is removed then changes have to be made in that file. Note that these
classes call one of the `QueryRouter.submitQuery` which in turn uses this
function.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on PR #15388: URL: https://github.com/apache/pinot/pull/15388#issuecomment-2853254490 > Mostly good. Can you please rebase it to the latest master? Rebased. -- 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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2074260352
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+ private Map _offlineRoutingTable;
+ private Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo() {
+ }
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
Review Comment:
Annotate all arguments as `@Nullable`.
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,370 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private String _offlineTableName = null;
+ private boolean _isOfflineRouteExists;
+ private TableConfig _offlineTableConfig;
+ private boolean _isOfflineTableDisabled;
+
+ private String _realtimeTableName = null;
+ private boolean _isRealtimeRouteExists;
+ private TableConfig _realtimeTableConfig;
+ private boolean _isRealtimeTableDisabled;
+
+ private TimeBoundaryInfo _timeBoundaryInfo;
+
+ private List _unavailableSegments;
+ private int _numPrunedSegmentsTotal;
+
+ private BrokerRequest _offlineBrokerRequest;
+ private BrokerRequest _realtimeBrokerRequest;
+
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2070459386
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.query.routing.table;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
Review Comment:
Also smaller functions make better unit testing possible. Each of these
functions have 100+ unit tests each
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2070449814
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
+
+ /**
+ * Gets the broker request for the offline table, if available.
+ *
+ * @return the broker request for the offline table, or null if not available
+ */
+ @Nullable
+ BrokerRequest getOfflineBrokerRequest();
+
+ /**
+ * Gets the broker request for the realtime table, if available.
+ *
+ * @return the broker request for the realtime table, or null if not
available
+ */
+ @Nullable
+ BrokerRequest getRealtimeBrokerRequest();
+
+ /**
+ * Gets the table config for the offline table, if available.
+ * @return the table config for the offline table, or null if not available
+ */
+ @Nullable
+ TableConfig getOfflineTableConfig();
+
+ /**
+ * Gets the table config for the realtime table, if available.
+ * @return the table config for the realtime table, or null if not available
+ */
+ @Nullable
+ TableConfig getRealtimeTableConfig();
Review Comment:
Sure. I am working on a response to this slack conversation:
https://apache-pinot.slack.com/archives/C07TESEPS4A/p1745838450704719
That should resolve how to handle table configs.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2070439034
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.query.routing.table;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
Review Comment:
It has to be in two separate functions because the control flow goes back
and forth between the route provider and `BaseSingleStageBrokerRequestHandler`.
This conversation is highly relevant:
https://github.com/apache/pinot/pull/15388#discussion_r2056924754
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2070430576
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
Review Comment:
`LogicalTableRouteInfo` returns the logical table name with the right
suffixes. These names are eventually used to create offline & realtime broker
requests.
Check this draft PR for the implementation of that file:
https://github.com/apache/pinot/pull/15634/commits/b0859ebff7daeb07f4f4462d63c9899580565df0#diff-05f6cda0d6be5f155dfb376d0401fe69a2b01b1e1f45a891f65ded1201563ee6R192
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2070431750
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
Review Comment:
Check
https://github.com/apache/pinot/pull/15634/commits/b0859ebff7daeb07f4f4462d63c9899580565df0#diff-05f6cda0d6be5f155dfb376d0401fe69a2b01b1e1f45a891f65ded1201563ee6R192
Most of the functions have different implementations. The ones that are the
same are those that havent been "figured out" yet.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2069700732
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.query.routing.table;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
Review Comment:
Much better now. Do we still need 2 methods? Shall we merge it into 1?
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r206973
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
Review Comment:
The methods in this interface are closely tight to hybrid table, where there
is up to 1 OFFLINE table and 1 REALTIME table. Most of the methods do not apply
to the case of `m` OFFLINE tables and `n` REALTIME tables. That's why I'm
asking should we just keep a concrete class for hybrid table (essentially the
refactoring) without introducing this interface
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2069697456
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
+
+ /**
+ * Gets the broker request for the offline table, if available.
+ *
+ * @return the broker request for the offline table, or null if not available
+ */
+ @Nullable
+ BrokerRequest getOfflineBrokerRequest();
+
+ /**
+ * Gets the broker request for the realtime table, if available.
+ *
+ * @return the broker request for the realtime table, or null if not
available
+ */
+ @Nullable
+ BrokerRequest getRealtimeBrokerRequest();
+
+ /**
+ * Gets the table config for the offline table, if available.
+ * @return the table config for the offline table, or null if not available
+ */
+ @Nullable
+ TableConfig getOfflineTableConfig();
+
+ /**
+ * Gets the table config for the realtime table, if available.
+ * @return the table config for the realtime table, or null if not available
+ */
+ @Nullable
+ TableConfig getRealtimeTableConfig();
Review Comment:
We can keep the intermediate code as needed. Just want to make sure we know
the end state and a path moving there
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2069696864
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
Review Comment:
For a logical table with `m` OFFLINE tables and `n` REALTIME tables, what
should we return here?
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057663499
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
+
+ /**
+ * Gets the broker request for the offline table, if available.
+ *
+ * @return the broker request for the offline table, or null if not available
+ */
+ @Nullable
+ BrokerRequest getOfflineBrokerRequest();
+
+ /**
+ * Gets the broker request for the realtime table, if available.
+ *
+ * @return the broker request for the realtime table, or null if not
available
+ */
+ @Nullable
+ BrokerRequest getRealtimeBrokerRequest();
+
+ /**
+ * Gets the table config for the offline table, if available.
+ * @return the table config for the offline table, or null if not available
+ */
+ @Nullable
+ TableConfig getOfflineTableConfig();
+
+ /**
+ * Gets the table config for the realtime table, if available.
+ * @return the table config for the realtime table, or null if not available
+ */
+ @Nullable
+ TableConfig getRealtimeTableConfig();
Review Comment:
You are right. Only Query & Routing configs are required from table configs.
Once https://github.com/apache/pinot/issues/15607 is implemented, we can only
return only those configs. Is it OK to keep it for now ? I can add TODO if
necessary.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057511664
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
Review Comment:
Unfortunately the fact that these are null or not is a core requirement of
the code in `BaseSingleStageBrokerRequestHandler`. So it does apply to logical
tables. In logical tables, the logical table name with type OR null is returned.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057658818
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,49 @@
+/**
+ * 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.query.routing.table;
+
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.transport.ImplicitHybridTableRouteInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
+ ImplicitHybridTableRouteInfo getTableRouteInfo(String tableName, TableCache
tableCache,
Review Comment:
`getTableRoute` gets and fills metadata into a `TableRouteInfo` object. It
also does some semantic checks. Then `calculateRoute` calculates the route
along with `BrokerRequest` that are created in
`BaseSingleStageBrokerRequestHandler`
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057656005
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,49 @@
+/**
+ * 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.query.routing.table;
+
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.transport.ImplicitHybridTableRouteInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
+ ImplicitHybridTableRouteInfo getTableRouteInfo(String tableName, TableCache
tableCache,
Review Comment:
Yes. Fixed
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057524176
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,49 @@
+/**
+ * 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.query.routing.table;
+
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.transport.ImplicitHybridTableRouteInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
+ ImplicitHybridTableRouteInfo getTableRouteInfo(String tableName, TableCache
tableCache,
Review Comment:
(please let me rant a bit :) )
The split into 3 pieces is by design. I have spent 90% of the effort to
understand this function and surgically move the workflow in and out of these 3
parts (Also moving out compile & the failed attempt at
https://github.com/apache/pinot/pull/15240)
The two major issues are:
* This function is a monster with spaghetti like workflow and a decade of
assumptions and organic growth.
* There are no unit tests.
I have tried to reduce the impact of the refactor in two ways:
* Only rewrite code that is required. Code untouched is code not broken.
* Add *lots* of tests for the code I did refactor. I have written or
generated 200 unit tests for `getTableRouteInfo()` and `calculateRoutes()`. I
have definitely broken something. With the new test framework I should be able
to quickly reproduce and add a test so that modifications wont lead to repeated
mistakes.
If I had to move the rest of the code, I will have to put the same effort to
test it. I dont see the value in that at this moment.
While the split into 3 parts may not look great from an absolute code
quality review, IMO this is much better than the previous state. Two critical
parts have clean separations, has a test framework and has a good canon of
tests.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057512832
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
Review Comment:
Yes. The two different implementations of `getRequestMap` is one of the core
pieces of logical table support.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2057511664
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
+
+ @Nullable
+ String getOfflineTableName();
+
+ @Nullable
+ String getRealtimeTableName();
Review Comment:
Unfortunately the fact that these are null or not is a core requirement of
the code in `BaseSingleStageBrokerRequestHandler`. So it does apply to logical
tables
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on PR #15388: URL: https://github.com/apache/pinot/pull/15388#issuecomment-2826348084 > `TableRouteInfo` is not really general. Is it introduced just for refactoring purpose? If so, let's only add the implementation for now without the interface because interface should be the general abstraction which also applies to the logical table It does apply to LogicalTable as well. Also an interface is required because `TableRouteInfo.getRequestMap` have different implementations and the two different implementations of that function is the core piece of that support. I can walk you through the implementation for logical tables if that will help. -- 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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2056924417
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,49 @@
+/**
+ * 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.query.routing.table;
+
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.transport.ImplicitHybridTableRouteInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
+ ImplicitHybridTableRouteInfo getTableRouteInfo(String tableName, TableCache
tableCache,
Review Comment:
Should this return `TableRouteInfo`?
##
pinot-core/src/main/java/org/apache/pinot/core/transport/TableRouteInfo.java:
##
@@ -0,0 +1,198 @@
+/**
+ * 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.transport;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * Routing information for a table required to route plan fragments to servers.
+ * RequestMap is used to submit plan fragments to servers.
+ * Offline and realtime routing tables are used to keep track of which servers
are executing the query.
+ * getUnavailableSegments() returns the segments that are not available when
calculating the routing table.
+ * numPrunedSegmentsTotal() returns the number of segments that were pruned
when calculating the routing table.
+ */
+public interface TableRouteInfo {
Review Comment:
Do we need an interface for the route info? I feel it should be a concrete
class with just information needed for routing purpose
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,49 @@
+/**
+ * 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.query.routing.table;
+
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.B
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on PR #15388: URL: https://github.com/apache/pinot/pull/15388#issuecomment-2824949540 Test failure is not related to the PR -- 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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2055752097
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.query.routing.table;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
Review Comment:
Done. Please check now
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
Jackie-Jiang commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2055048738
##
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/table/TableRouteProvider.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.query.routing.table;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.core.routing.RoutingManager;
+import org.apache.pinot.core.routing.TimeBoundaryInfo;
+import org.apache.pinot.core.transport.TableRouteInfo;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+
+/**
+ * The TableRoute interface provides the metadata required to route query
execution to servers. The important sources
+ * of the metadata are table config, broker routing information and the broker
request.
+ */
+public interface TableRouteProvider {
Review Comment:
Currently the functionality of this interface is mixed with
`TableRouteInfo`, where both of them contains routing info.
Ideally this interface should encapsulate the logic of computing routing
info, and should be re-usable across queries. All the routing info should be
stored within `TableRouteInfo`. With the current PR, half of the info are
stored in the provider and half stored in `TableRouteInfo`.
Can you please try to split the computing logic and result, and only keep
the logic in the provider (maybe rename it to computer), and the info in
`TableRouteInfo`? One principal to help you with the split is to see if you can
reuse the computing logic across queries.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
vrajat commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2036916635
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private final BrokerRequest _offlineBrokerRequest;
+ private final BrokerRequest _realtimeBrokerRequest;
+ private final Map _offlineRoutingTable;
+ private final Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
+ Map offlineRoutingTable,
+ Map realtimeRoutingTable) {
+_offlineBrokerRequest = offlineBrokerRequest;
+_realtimeBrokerRequest = realtimeBrokerRequest;
+_offlineRoutingTable = offlineRoutingTable;
+_realtimeRoutingTable = realtimeRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getOfflineBrokerRequest() {
+return _offlineBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getRealtimeBrokerRequest() {
+return _realtimeBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public Map getOfflineRoutingTable() {
+return _offlineRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public Map getRealtimeRoutingTable() {
+return _realtimeRoutingTable;
+ }
+
+ protected static Map
getRequestMapFromRoutingTable(TableType tableType,
+ Map routingTable, BrokerRequest
brokerRequest, long requestId, String brokerId,
+ boolean preferTls) {
+Map requestMap = new HashMap<>();
+for (Map.Entry entry :
routingTable.entrySet()) {
+ ServerRoutingInstance serverRoutingInstance =
entry.getKey().toServerRoutingInstance(tableType, preferTls);
+ InstanceRequest instanceRequest = getInstanceRequest(requestId,
brokerId, brokerRequest, entry.getValue());
+ requestMap.put(serverRoutingInstance, instanceRequest);
+}
+return requestMap;
+ }
+
+ protected static InstanceRequest getInstanceRequest(long requestId, String
brokerId, BrokerRequest brokerRequest,
+ ServerRouteInfo segments) {
+InstanceRequest instanceRequest = new InstanceRequest();
+instanceRequest.setRequestId(requestId);
+instanceRequest.setCid(QueryThreadContext.getCid());
+instanceRequest.setQuery(brokerRequest);
+Map queryOptions =
brokerRequest.getPinotQuery().getQueryOptions();
+if (queryOptions != null) {
+
instanceRequest.setEnableTrace(Boolean.parseBoolean(queryOptions.get(CommonConstants.Broker.Request.TRACE)));
+}
+instanceRequest.setSearchSegments(segments.getSegments());
+instanceRequest.setBrokerId(brokerId);
+if (CollectionUtils.isNotEmpty(segments.getOptionalSegments())) {
+ // Don't set this field, i.e. leave it as null, if there is no optional
segment at all, to be more backward
+ // compatible, as there are places like in multi-stage query engine
where this field is not set today when
+ // creating the InstanceRequest.
+ instanceRequest.setOptionalSegments(segments.getOptionalSegments());
+}
+return instanceRequest;
+ }
+
+ public Map getRequestMap(long
requestId, String brokerId, boolean preferTls) {
Review Comment:
I didnt do that to avoid creating objects. The current code also uses null
checks intsead of empty. Also this code is in performance critical path, I've
tried to avoid creating objects.
--
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.
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
abhishekbafna commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2036469268
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private final BrokerRequest _offlineBrokerRequest;
+ private final BrokerRequest _realtimeBrokerRequest;
+ private final Map _offlineRoutingTable;
+ private final Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
+ Map offlineRoutingTable,
+ Map realtimeRoutingTable) {
+_offlineBrokerRequest = offlineBrokerRequest;
+_realtimeBrokerRequest = realtimeBrokerRequest;
+_offlineRoutingTable = offlineRoutingTable;
+_realtimeRoutingTable = realtimeRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getOfflineBrokerRequest() {
+return _offlineBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getRealtimeBrokerRequest() {
+return _realtimeBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public Map getOfflineRoutingTable() {
+return _offlineRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public Map getRealtimeRoutingTable() {
+return _realtimeRoutingTable;
+ }
+
+ protected static Map
getRequestMapFromRoutingTable(TableType tableType,
+ Map routingTable, BrokerRequest
brokerRequest, long requestId, String brokerId,
+ boolean preferTls) {
+Map requestMap = new HashMap<>();
+for (Map.Entry entry :
routingTable.entrySet()) {
+ ServerRoutingInstance serverRoutingInstance =
entry.getKey().toServerRoutingInstance(tableType, preferTls);
+ InstanceRequest instanceRequest = getInstanceRequest(requestId,
brokerId, brokerRequest, entry.getValue());
+ requestMap.put(serverRoutingInstance, instanceRequest);
+}
+return requestMap;
+ }
+
+ protected static InstanceRequest getInstanceRequest(long requestId, String
brokerId, BrokerRequest brokerRequest,
+ ServerRouteInfo segments) {
+InstanceRequest instanceRequest = new InstanceRequest();
+instanceRequest.setRequestId(requestId);
+instanceRequest.setCid(QueryThreadContext.getCid());
+instanceRequest.setQuery(brokerRequest);
+Map queryOptions =
brokerRequest.getPinotQuery().getQueryOptions();
+if (queryOptions != null) {
+
instanceRequest.setEnableTrace(Boolean.parseBoolean(queryOptions.get(CommonConstants.Broker.Request.TRACE)));
+}
+instanceRequest.setSearchSegments(segments.getSegments());
+instanceRequest.setBrokerId(brokerId);
+if (CollectionUtils.isNotEmpty(segments.getOptionalSegments())) {
+ // Don't set this field, i.e. leave it as null, if there is no optional
segment at all, to be more backward
+ // compatible, as there are places like in multi-stage query engine
where this field is not set today when
+ // creating the InstanceRequest.
+ instanceRequest.setOptionalSegments(segments.getOptionalSegments());
+}
+return instanceRequest;
+ }
+
+ public Map getRequestMap(long
requestId, String brokerId, boolean preferTls) {
Review Comment:
The method can be simplified with empty init of the request map and adding
the request map using putAll.
--
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 Infrastr
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
abhishekbafna commented on code in PR #15388:
URL: https://github.com/apache/pinot/pull/15388#discussion_r2036469268
##
pinot-core/src/main/java/org/apache/pinot/core/transport/ImplicitHybridTableRouteInfo.java:
##
@@ -0,0 +1,135 @@
+/**
+ * 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.transport;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.InstanceRequest;
+import org.apache.pinot.core.routing.ServerRouteInfo;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.query.QueryThreadContext;
+import org.apache.pinot.spi.utils.CommonConstants;
+
+
+public class ImplicitHybridTableRouteInfo implements TableRouteInfo {
+ private final BrokerRequest _offlineBrokerRequest;
+ private final BrokerRequest _realtimeBrokerRequest;
+ private final Map _offlineRoutingTable;
+ private final Map _realtimeRoutingTable;
+
+ public ImplicitHybridTableRouteInfo(BrokerRequest offlineBrokerRequest,
BrokerRequest realtimeBrokerRequest,
+ Map offlineRoutingTable,
+ Map realtimeRoutingTable) {
+_offlineBrokerRequest = offlineBrokerRequest;
+_realtimeBrokerRequest = realtimeBrokerRequest;
+_offlineRoutingTable = offlineRoutingTable;
+_realtimeRoutingTable = realtimeRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getOfflineBrokerRequest() {
+return _offlineBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public BrokerRequest getRealtimeBrokerRequest() {
+return _realtimeBrokerRequest;
+ }
+
+ @Nullable
+ @Override
+ public Map getOfflineRoutingTable() {
+return _offlineRoutingTable;
+ }
+
+ @Nullable
+ @Override
+ public Map getRealtimeRoutingTable() {
+return _realtimeRoutingTable;
+ }
+
+ protected static Map
getRequestMapFromRoutingTable(TableType tableType,
+ Map routingTable, BrokerRequest
brokerRequest, long requestId, String brokerId,
+ boolean preferTls) {
+Map requestMap = new HashMap<>();
+for (Map.Entry entry :
routingTable.entrySet()) {
+ ServerRoutingInstance serverRoutingInstance =
entry.getKey().toServerRoutingInstance(tableType, preferTls);
+ InstanceRequest instanceRequest = getInstanceRequest(requestId,
brokerId, brokerRequest, entry.getValue());
+ requestMap.put(serverRoutingInstance, instanceRequest);
+}
+return requestMap;
+ }
+
+ protected static InstanceRequest getInstanceRequest(long requestId, String
brokerId, BrokerRequest brokerRequest,
+ ServerRouteInfo segments) {
+InstanceRequest instanceRequest = new InstanceRequest();
+instanceRequest.setRequestId(requestId);
+instanceRequest.setCid(QueryThreadContext.getCid());
+instanceRequest.setQuery(brokerRequest);
+Map queryOptions =
brokerRequest.getPinotQuery().getQueryOptions();
+if (queryOptions != null) {
+
instanceRequest.setEnableTrace(Boolean.parseBoolean(queryOptions.get(CommonConstants.Broker.Request.TRACE)));
+}
+instanceRequest.setSearchSegments(segments.getSegments());
+instanceRequest.setBrokerId(brokerId);
+if (CollectionUtils.isNotEmpty(segments.getOptionalSegments())) {
+ // Don't set this field, i.e. leave it as null, if there is no optional
segment at all, to be more backward
+ // compatible, as there are places like in multi-stage query engine
where this field is not set today when
+ // creating the InstanceRequest.
+ instanceRequest.setOptionalSegments(segments.getOptionalSegments());
+}
+return instanceRequest;
+ }
+
+ public Map getRequestMap(long
requestId, String brokerId, boolean preferTls) {
Review Comment:
The can be simplified with empty init of the request map and adding the
request map using putAll.
--
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
Re: [PR] Move Table Routing logic in SSE to TableRoute class [pinot]
codecov-commenter commented on PR #15388: URL: https://github.com/apache/pinot/pull/15388#issuecomment-2786231436 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15388?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `79.75460%` with `33 lines` in your changes missing coverage. Please review. > Project coverage is 55.85%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`1be8502`)](https://app.codecov.io/gh/apache/pinot/commit/1be8502c113bb93a3e0626691276f5c0f8d33997?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 2012 commits behind head on master. | [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15388?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [.../pinot/query/table/ImplicitTableRouteComputer.java](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Ftable%2FImplicitTableRouteComputer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdGFibGUvSW1wbGljaXRUYWJsZVJvdXRlQ29tcHV0ZXIuamF2YQ==) | 84.21% | [14 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...a/org/apache/pinot/core/transport/QueryRouter.java](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FQueryRouter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvUXVlcnlSb3V0ZXIuamF2YQ==) | 25.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...g/apache/pinot/core/transport/TableRouteUtils.java](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2FTableRouteUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvVGFibGVSb3V0ZVV0aWxzLmphdmE=) | 86.11% | [1 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [.../org/apache/pinot/core/routing/RoutingManager.java](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Frouting%2FRoutingManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdNYW5hZ2VyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | > :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (1be8502). Click for more details. > > HEAD has 48 uploads less than BASE > >| Flag | BASE (59551e4) | HEAD (1be8502) | >|--|--|--| >|integration|7|0| >|integration2|3|0| >|temurin|12|2| >|java-21|7|2| >|skip-bytebuffers-true|3|1| >|skip-bytebuffers-false|7|1| >|unittests|5|2| >|java-11|5|0| >|unittests2|3|0| >|integration1|2|0| >|custom-integration1|2|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #15388 +/- ## - Coverage 61.75% 55.85% -5.90% - Complexity 207 681 +474 Files 2436 2226 -210 Lines133233 120196 -13037 Branches 2063619208-1428 - Hits 8227467
