[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5483: Add QueryRequest to replace BrokerRequest in query execution engine
mayankshriv commented on a change in pull request #5483:
URL: https://github.com/apache/incubator-pinot/pull/5483#discussion_r434318564
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/v2/utils/BrokerRequestToQueryRequestConverter.java
##
@@ -0,0 +1,154 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.request.v2.utils;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.common.request.AggregationInfo;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.request.FilterOperator;
+import org.apache.pinot.common.request.GroupBy;
+import org.apache.pinot.common.request.Selection;
+import org.apache.pinot.common.request.SelectionSort;
+import org.apache.pinot.common.request.v2.Expression;
+import org.apache.pinot.common.request.v2.FilterFunction;
+import org.apache.pinot.common.request.v2.Function;
+import org.apache.pinot.common.request.v2.OrderByExpression;
+import org.apache.pinot.common.request.v2.QueryRequest;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.pql.parsers.Pql2Compiler;
+import org.apache.pinot.pql.parsers.pql2.ast.AstNode;
+import org.apache.pinot.pql.parsers.pql2.ast.FunctionCallAstNode;
+import org.apache.pinot.pql.parsers.pql2.ast.IdentifierAstNode;
+import org.apache.pinot.pql.parsers.pql2.ast.LiteralAstNode;
+
+
+public class BrokerRequestToQueryRequestConverter {
Review comment:
Isn't the damage already done when an expression from PinotQuery was
converted to a string in BrokerRequest? Can we always get back the original
expression from the string inside BrokerRequest (I recently ran into an issue
where we lost the info whether a token was literal or not, will dig up the
details).
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.
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
[GitHub] [incubator-pinot] mayankshriv commented on pull request #5483: Add QueryRequest to replace BrokerRequest in query execution engine
mayankshriv commented on pull request #5483: URL: https://github.com/apache/incubator-pinot/pull/5483#issuecomment-637964051 Should we solve the issues in BrokerRequest, or move to PinotQuery? Latter would be ideal, except in organizations with large Pinot deployments, moving to SQL may not happen immediately. This implies, that we would need to build PinotQuery out of brokerRequest (instead of the other way round). 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. 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
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5461: Adding Support for SQL CASE Statement
mayankshriv commented on a change in pull request #5461:
URL: https://github.com/apache/incubator-pinot/pull/5461#discussion_r434311036
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java
##
@@ -0,0 +1,320 @@
+/**
+ * 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.operator.transform.function;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.core.common.DataSource;
+import org.apache.pinot.core.operator.blocks.ProjectionBlock;
+import org.apache.pinot.core.operator.transform.TransformResultMetadata;
+import org.apache.pinot.core.plan.DocIdSetPlanNode;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.utils.ByteArray;
+
+
+/**
+ * BinaryOperatorTransformFunction abstracts common functions for
binary operators (=, !=, >=, >, <=, <)
+ * The results are in boolean format and stored as an integer array with 1
represents true and 0 represents false.
+ */
+public abstract class BinaryOperatorTransformFunction extends
BaseTransformFunction {
+
+ protected TransformFunction _leftTransformFunction;
+ protected TransformFunction _rightTransformFunction;
+ protected FieldSpec.DataType _leftDataType;
+ protected FieldSpec.DataType _rightDataType;
+ protected int[] _results;
+
+ @Override
+ public void init(List arguments, Map
dataSourceMap) {
+// Check that there are exact 2 arguments
+if (arguments.size() != 2) {
Review comment:
Any reason to not use Preconditions.checkArgument()?
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java
##
@@ -0,0 +1,320 @@
+/**
+ * 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.operator.transform.function;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.core.common.DataSource;
+import org.apache.pinot.core.operator.blocks.ProjectionBlock;
+import org.apache.pinot.core.operator.transform.TransformResultMetadata;
+import org.apache.pinot.core.plan.DocIdSetPlanNode;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.utils.ByteArray;
+
+
+/**
+ * BinaryOperatorTransformFunction abstracts common functions for
binary operators (=, !=, >=, >, <=, <)
+ * The results are in boolean format and stored as an integer array with 1
represents true and 0 represents false.
+ */
+public abstract class BinaryOperatorTransformFunction extends
BaseTransformFunction {
+
+ protected TransformFunction _leftTransformFunction;
+ protected TransformFunction _rightTransformFunction;
+ protected FieldSpec.DataType _leftDataType;
+ protected FieldSpec.DataType _rightDataType;
+ protected int[] _results;
+
+ @Override
+ public void init(List arguments, Map
dataSourceMap) {
+// Check that there are exact 2 arguments
+if (arguments.size() != 2) {
+ throw new IllegalArgumentException("Exact 2 arguments are required for
binary operator transform function");
+}
+_leftTransformFunction = arguments.get(0);
+_rightTransformFunction = arguments.get(1);
+_leftDataType = _leftTransformFunction.getResultMetadata().getDataType();
+_rightDataType = _rightTransformFunction.getResultMetadata().getDataType();
+switch (_leftDataType) {
Review
[GitHub] [incubator-pinot] yupeng9 commented on pull request #5487: Add multi-value support to SegmentDumpTool
yupeng9 commented on pull request #5487: URL: https://github.com/apache/incubator-pinot/pull/5487#issuecomment-637961796 @mayankshriv That's true for better code reuse. Though this PR is for a minor improvement, and I believe we could the refactoring in another 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. 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
[GitHub] [incubator-pinot] yupeng9 commented on a change in pull request #5487: Add multi-value support to SegmentDumpTool
yupeng9 commented on a change in pull request #5487:
URL: https://github.com/apache/incubator-pinot/pull/5487#discussion_r434310701
##
File path: pinot-tools/src/main/java/org/apache/pinot/tools/SegmentDumpTool.java
##
@@ -91,11 +102,27 @@ public void doMain(String[] args)
System.out.print(i);
System.out.print("\t");
for (String columnName : columnNames) {
-BlockSingleValIterator itr = iterators.get(columnName);
-int encodedValue = itr.nextIntVal();
-Object value = dictionaries.get(columnName).get(encodedValue);
-System.out.print(value);
-System.out.print("\t");
+BlockValIterator itr = iterators.get(columnName);
+if (itr instanceof BlockSingleValIterator) {
+ int encodedValue = ((BlockSingleValIterator) itr).nextIntVal();
+ Object value = dictionaries.get(columnName).get(encodedValue);
+ System.out.print(value);
+ System.out.print("\t");
+} else {
+ BlockMultiValIterator mItr = (BlockMultiValIterator) itr;
+ int maxNumValuesPerMVEntry =
+
indexSegment.getDataSource(columnName).getDataSourceMetadata().getMaxNumValuesPerMVEntry();
+ int[] intArray = new int[maxNumValuesPerMVEntry];
+ int length = mItr.nextIntVal(intArray);
+ System.out.print("[");
+ for (int j = 0; j < length; j++) {
+System.out.print(dictionaries.get(columnName).get(intArray[j]));
+if (j != length - 1) {
+ System.out.print(",");
+}
+ }
+ System.out.print("]");
Review comment:
good catch
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.
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
[GitHub] [incubator-pinot] npawar commented on issue #5390: Improve the experience when adding indexes and reloading
npawar commented on issue #5390: URL: https://github.com/apache/incubator-pinot/issues/5390#issuecomment-637941159 I think one approach could be as simple as: We introduce an API for reading physical segment's metadata files `/segments/index-metadata` (need a better name) 1. controller untars the segment 2. reads the metadata using SegmentMetadataImpl 3. extracts needed info from metadata.properties and indexMap 4. deletes uncompressed segment 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. 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
[GitHub] [incubator-pinot] fx19880617 opened a new pull request #5489: Upgrade pinotdb version to 0.3.1 to use new pinot sql api
fx19880617 opened a new pull request #5489: URL: https://github.com/apache/incubator-pinot/pull/5489 ## Description Upgrade pypi pinotdb(https://pypi.org/project/pinotdb/) version to 0.3.1 to use new pinot sql api. ## Release Notes No 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. 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
[incubator-pinot] branch upgrade-superset-pinotdb-version created (now 9d9df06)
This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a change to branch upgrade-superset-pinotdb-version in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. at 9d9df06 Upgrade pinotdb version to 0.3.1 to use new pinot sql api No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch upgrade-superset-pinotdb-version created (now 9d9df06)
This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a change to branch upgrade-superset-pinotdb-version in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. at 9d9df06 Upgrade pinotdb version to 0.3.1 to use new pinot sql api No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5487: Add multi-value support to SegmentDumpTool
fx19880617 commented on a change in pull request #5487:
URL: https://github.com/apache/incubator-pinot/pull/5487#discussion_r434288816
##
File path: pinot-tools/src/main/java/org/apache/pinot/tools/SegmentDumpTool.java
##
@@ -91,11 +102,27 @@ public void doMain(String[] args)
System.out.print(i);
System.out.print("\t");
for (String columnName : columnNames) {
-BlockSingleValIterator itr = iterators.get(columnName);
-int encodedValue = itr.nextIntVal();
-Object value = dictionaries.get(columnName).get(encodedValue);
-System.out.print(value);
-System.out.print("\t");
+BlockValIterator itr = iterators.get(columnName);
+if (itr instanceof BlockSingleValIterator) {
+ int encodedValue = ((BlockSingleValIterator) itr).nextIntVal();
+ Object value = dictionaries.get(columnName).get(encodedValue);
+ System.out.print(value);
+ System.out.print("\t");
+} else {
+ BlockMultiValIterator mItr = (BlockMultiValIterator) itr;
+ int maxNumValuesPerMVEntry =
+
indexSegment.getDataSource(columnName).getDataSourceMetadata().getMaxNumValuesPerMVEntry();
+ int[] intArray = new int[maxNumValuesPerMVEntry];
+ int length = mItr.nextIntVal(intArray);
+ System.out.print("[");
+ for (int j = 0; j < length; j++) {
+System.out.print(dictionaries.get(columnName).get(intArray[j]));
+if (j != length - 1) {
+ System.out.print(",");
+}
+ }
+ System.out.print("]");
Review comment:
add a `System.out.print("\t");`?
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.
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
[GitHub] [incubator-pinot] jackjlli opened a new pull request #5488: Move bintray target to mvn deploy command
jackjlli opened a new pull request #5488: URL: https://github.com/apache/incubator-pinot/pull/5488 This PR moves LinkedIn bintray target as a parameter in `mvn deploy` command. Reference: https://stackoverflow.com/questions/12435283/adding-a-maven-distribution-repository-on-the-command-line 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. 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
[incubator-pinot] 01/01: Remove bintray target to mvn deploy command
This is an automated email from the ASF dual-hosted git repository.
jlli pushed a commit to branch bintray-cli
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
commit 870b0b99a3d84d43b0dcc1dff8369c74bbd87fa0
Author: Jack Li(Analytics Engineering)
AuthorDate: Tue Jun 2 20:29:31 2020 -0700
Remove bintray target to mvn deploy command
---
.travis/.travis_nightly_build.sh | 2 +-
pom.xml | 8
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/.travis/.travis_nightly_build.sh b/.travis/.travis_nightly_build.sh
index 461b8a9..2fed878 100755
--- a/.travis/.travis_nightly_build.sh
+++ b/.travis/.travis_nightly_build.sh
@@ -27,5 +27,5 @@ if [ -n "${DEPLOY_BUILD_OPTS}" ]; then
mvn versions:commit -q -B
# Deploy to bintray
- mvn deploy -s .travis/.ci.settings.xml -DskipTests -q
-DretryFailedDeploymentCount=5
+ mvn deploy -s .travis/.ci.settings.xml -DskipTests -q
-DretryFailedDeploymentCount=5
-DaltDeploymentRepository=bintray-linkedin-maven::default::'https://api.bintray.com/maven/linkedin/maven/pinot/;publish=1;override=1'
fi
diff --git a/pom.xml b/pom.xml
index c9b8d23..da5088e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -97,14 +97,6 @@
2018
-
-
- bintray-linkedin-maven
- linkedin-maven
-
https://api.bintray.com/maven/linkedin/maven/pinot/;publish=1;override=1
-
-
-
${basedir}
0.4.0
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[incubator-pinot] branch bintray-cli created (now 870b0b9)
This is an automated email from the ASF dual-hosted git repository. jlli pushed a change to branch bintray-cli in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. at 870b0b9 Remove bintray target to mvn deploy command This branch includes the following new commits: new 870b0b9 Remove bintray target to mvn deploy command The 1 revisions listed above as "new" are entirely new to this repository and will be described in separate emails. The revisions listed as "add" were already present in the repository and have only been added to this reference. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] guruguha commented on issue #5390: Improve the experience when adding indexes and reloading
guruguha commented on issue #5390: URL: https://github.com/apache/incubator-pinot/issues/5390#issuecomment-637918220 I have an initial approach to this issue: We can introduce a segment state monitor that keeps track of all the segments and their index version updates. Similar to `_lastKnownSegmentMetadataVersionMap`, but it is updated whenever a segment index is updated. The monitor could be the callback passed as part of the segment reload - currently there is none. On the API front, - provide an end point for users to query for segment status - get the version change status from the resource manager - parse the SegmentZKMetadata object to return the value as a JSON string Please let me know your thoughts and inputs. 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. 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
[GitHub] [incubator-pinot] yupeng9 opened a new pull request #5487: Add multi-value support to SegmentDumpTool
yupeng9 opened a new pull request #5487: URL: https://github.com/apache/incubator-pinot/pull/5487 Also, add the segment dump tool as part of the pinot-tool.sh script Currently segment dump tool supports single-value only. This change adds the multi-value support. It also makes segment dump tool a subcommand in `pinot-tool.sh` script. 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. 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
[GitHub] [incubator-pinot] guruguha removed a comment on issue #5390: Improve the experience when adding indexes and reloading
guruguha removed a comment on issue #5390: URL: https://github.com/apache/incubator-pinot/issues/5390#issuecomment-637914979 I have an initial approach to this issue: - provide an end point for users to query for segment status - the end point would need to query the ZK to get the input segment metadata based on the type of the table - parse the `SegmentZKMetadata` object to return the value as a JSON string 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. 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
[GitHub] [incubator-pinot] guruguha commented on issue #5390: Improve the experience when adding indexes and reloading
guruguha commented on issue #5390: URL: https://github.com/apache/incubator-pinot/issues/5390#issuecomment-637914979 I have an initial approach to this issue: - provide an end point for users to query for segment status - the end point would need to query the ZK to get the input segment metadata based on the type of the table - parse the `SegmentZKMetadata` object to return the value as a JSON string 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. 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
[GitHub] [incubator-pinot] kishoreg commented on pull request #5440: Add GenericTransformFunction wrapper for simple ScalarFunctions
kishoreg commented on pull request #5440: URL: https://github.com/apache/incubator-pinot/pull/5440#issuecomment-637897871 @sidd only for scalarfunctions. 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. 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
[GitHub] [incubator-pinot] siddharthteotia commented on pull request #5440: Add GenericTransformFunction wrapper for simple ScalarFunctions
siddharthteotia commented on pull request #5440: URL: https://github.com/apache/incubator-pinot/pull/5440#issuecomment-637875934 Is the plan to use this wrapper solely for invoking scalar functions (like already done in this PR for StringFunctions) or are we expecting follow-ups to integrate it with rest of the transform functions. I think only the former? 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. 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
[GitHub] [incubator-pinot] jackjlli commented on issue #5485: Remove LinkedIn config from pom
jackjlli commented on issue #5485: URL: https://github.com/apache/incubator-pinot/issues/5485#issuecomment-637868513 @haibow Thanks for the heads up. I can work on this. 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. 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
[GitHub] [incubator-pinot] jamesyfshao commented on pull request #5394: add callback interface for upsert component
jamesyfshao commented on pull request #5394: URL: https://github.com/apache/incubator-pinot/pull/5394#issuecomment-637852570 > @jamesyfshao I looked over your design doc, thanks for adding the section on interfaces you need. I also looked at the repo you mentioned. > It is not clear why we need all these interfaces. If you can add some clarifications in the design doc, that will help me better. We can then discuss online what the interface should be. > @kishoreg if you have additional comments @mcvsubbu I totally got why you feel that way as it is hard to lay out the all the exact details in without the code context. It could be confusing when we leave out the upsert-enabled callback implementation which shows why we need them. But please understand that we leave out some of exact interface details in the design doc as we are trying to make the design doc to be flexible - code could changes/evolve over time but the design doc should capture the high-level design ideals and data flows that will be consistent over the evolution of this feature, especially considered these interface is still being involved over the next few PRs. Do you think it might be helpful if I put more comments and example in the codes (mainly the interface method) to show why those new callbacks are added and their purpose instead in the design doc? This way we can update the documentation as we evolve our interface while keeping our design doc less noisy? 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. 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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5483: Add QueryRequest to replace BrokerRequest in query execution engine
Jackie-Jiang commented on a change in pull request #5483:
URL: https://github.com/apache/incubator-pinot/pull/5483#discussion_r434193068
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/v2/ServerQuery.java
##
@@ -0,0 +1,181 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.request.v2;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+public class ServerQuery {
Review comment:
@kishoreg After reconsideration, changed it to QueryRequest and use it
on both Broker and Server side. We can use a wrapper class on server side (E.g.
ServerQueryRequest) to store the server specific helper variables.
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.
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
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5336: [Part 4] Deep-store bypass for LLC: Add a peer to peer segment fetcher.
mcvsubbu commented on a change in pull request #5336:
URL: https://github.com/apache/incubator-pinot/pull/5336#discussion_r434191769
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/PeerServerSegmentFetcher.java
##
@@ -0,0 +1,155 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils.fetcher;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Random;
+import org.apache.commons.configuration.Configuration;
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.common.utils.LLCSegmentName;
+import org.apache.pinot.common.utils.StringUtil;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+
+/**
+ * This segment fetcher downloads the segment by first discovering the server
having the segment through external view
+ * of a Pinot table and then downloading the segment from the peer server
using a configured http or https fetcher. By
+ * default, HttpSegmentFetcher is used.
+ * The format fo expected segment address uri is
+ *peer:///segment_name
+ * Note the host component is empty.
+ */
+public class PeerServerSegmentFetcher extends BaseSegmentFetcher {
+ private static final String PEER_2_PEER_PROTOCOL = "peer";
+ private static final String PEER_SEGMENT_DOWNLOAD_SCHEME =
"peerSegmentDownloadScheme";
+ private HelixManager _helixManager;
+ private String _helixClusterName;
+ private HttpSegmentFetcher _httpSegmentFetcher;
+ // The value is either https or http
+ private final String _httpScheme;
+
+ public PeerServerSegmentFetcher(Configuration config, HelixManager
helixManager, String helixClusterName) {
+_helixManager = helixManager;
+_helixClusterName = helixClusterName;
+switch (config.getString(PEER_SEGMENT_DOWNLOAD_SCHEME)) {
+ case "https":
+_httpSegmentFetcher = new HttpsSegmentFetcher();
+_httpScheme = "https";
+break;
+ default:
+_httpSegmentFetcher = new HttpSegmentFetcher();
+_httpScheme = "http";
+}
+_httpSegmentFetcher.init(config);
+ }
+
+ public PeerServerSegmentFetcher(HttpSegmentFetcher httpSegmentFetcher,
String httpScheme, HelixManager helixManager,
+ String helixClusterName) {
+_helixManager = helixManager;
+_helixClusterName = helixClusterName;
+_httpSegmentFetcher = httpSegmentFetcher;
+_httpScheme = httpScheme;
+ }
+
+ @Override
+ public void fetchSegmentToLocalWithoutRetry(URI uri, File dest)
+ throws Exception {
+if (!PEER_2_PEER_PROTOCOL.equals(uri.getScheme())) {
Review comment:
Not sure how the peer-2-peer protocol plays here. Caller can create an
http segment fetcher, or https segment fetcher based on the table config,
right? The only extra logic that the caller needs is the construction of a URL.
Maybe that can be in a utils class, or just in the caller as a method (if it is
used by exactly one caller)
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/fetcher/PeerServerSegmentFetcher.java
##
@@ -0,0 +1,155 @@
+/**
+ * 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
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5394: add callback interface for upsert component
mcvsubbu commented on pull request #5394: URL: https://github.com/apache/incubator-pinot/pull/5394#issuecomment-637813614 @jamesyfshao I looked over your design doc, thanks for adding the section on interfaces you need. I also looked at the repo you mentioned. It is not clear why we need all these interfaces. If you can add some clarifications in the design doc, that will help me better. We can then discuss online what the interface should be. @kishoreg if you have additional comments 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. 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
[incubator-pinot] branch master updated: Add a new table config field for peer segment download. (#5478)
This is an automated email from the ASF dual-hosted git repository.
mcvsubbu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 8a3eb43 Add a new table config field for peer segment download.
(#5478)
8a3eb43 is described below
commit 8a3eb4315df59d6d73a4ec11308d30eab1c51042
Author: Ting Chen
AuthorDate: Tue Jun 2 14:00:47 2020 -0700
Add a new table config field for peer segment download. (#5478)
* Add a new table config field for peer segment download.
* Address feedbacks.
* Update
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
Co-authored-by: Subbu Subramaniam
* Update
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
Co-authored-by: Subbu Subramaniam
Co-authored-by: Subbu Subramaniam
---
.../pinot/common/utils/config/TableConfigUtils.java| 18 ++
.../common/utils/config/TableConfigSerDeTest.java | 11 +++
.../table/SegmentsValidationAndRetentionConfig.java| 11 +++
.../pinot/spi/utils/builder/TableConfigBuilder.java| 7 +++
4 files changed, 47 insertions(+)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
index d93b24b..b7b7d52 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
@@ -181,9 +181,15 @@ public class TableConfigUtils {
* Validates the table config with the following rules:
*
* Text index column must be raw
+ * peerSegmentDownloadScheme in ValidationConfig must be http or
https
*
*/
public static void validate(TableConfig tableConfig) {
+validateFieldConfigList(tableConfig);
+validateValidationConfig(tableConfig);
+ }
+
+ private static void validateFieldConfigList(TableConfig tableConfig) {
List fieldConfigList = tableConfig.getFieldConfigList();
if (fieldConfigList != null) {
List noDictionaryColumns =
tableConfig.getIndexingConfig().getNoDictionaryColumns();
@@ -201,4 +207,16 @@ public class TableConfigUtils {
}
}
}
+
+ private static void validateValidationConfig(TableConfig tableConfig) {
+SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
+if (validationConfig != null) {
+ String peerSegmentDownloadScheme =
validationConfig.getPeerSegmentDownloadScheme();
+ if (peerSegmentDownloadScheme != null) {
+if (!"http".equalsIgnoreCase(peerSegmentDownloadScheme) &&
!"https".equalsIgnoreCase(peerSegmentDownloadScheme)) {
+ throw new IllegalStateException("Invalid value '" +
peerSegmentDownloadScheme + "' for peerSegmentDownloadScheme. Must be one of
http nor https" );
+}
+ }
+}
+ }
}
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
index 4d34012..d2d2ebf 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeTest.java
@@ -243,6 +243,17 @@ public class TableConfigSerDeTest {
checkTableConfigWithUpsertConfig(JsonUtils.stringToObject(tableConfig.toJsonString(),
TableConfig.class));
checkTableConfigWithUpsertConfig(TableConfigUtils.fromZNRecord(TableConfigUtils.toZNRecord(tableConfig)));
}
+{
+ // with SegmentsValidationAndRetentionConfig
+ TableConfig tableConfig =
tableConfigBuilder.setPeerSegmentDownloadScheme("http").build();
+
checkSegmentsValidationAndRetentionConfig(JsonUtils.stringToObject(tableConfig.toJsonString(),
TableConfig.class));
+
checkSegmentsValidationAndRetentionConfig(TableConfigUtils.fromZNRecord(TableConfigUtils.toZNRecord(tableConfig)));
+}
+ }
+
+ private void checkSegmentsValidationAndRetentionConfig(TableConfig
tableConfig) {
+// TODO validate other fields of SegmentsValidationAndRetentionConfig.
+
assertEquals(tableConfig.getValidationConfig().getPeerSegmentDownloadScheme(),
"http");
}
private void checkDefaultTableConfig(TableConfig tableConfig) {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
index 4e0f3e2..30451ab 100644
---
a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
+++
[GitHub] [incubator-pinot] mcvsubbu merged pull request #5478: Add a new table config field for peer segment download.
mcvsubbu merged pull request #5478: URL: https://github.com/apache/incubator-pinot/pull/5478 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. 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
[GitHub] [incubator-pinot] mcvsubbu opened a new pull request #5486: Changed the segment commit protocol to send/receive streamPartitionMs…
mcvsubbu opened a new pull request #5486: URL: https://github.com/apache/incubator-pinot/pull/5486 …gOffset Updated the segment commit protocol so that new element streamPartitionMsgOffset is populated in requests (as request parameters) and in response (as JSON string element) The server side has been modified to send the 'streamPartitionMsgOffset' as well as we the 'offset' parameters to the controller. The controller looks for and prefers streamPartitionMsgOffset but falls back to offset if the streamPartitionMsgOffset is not there. The controller side, in the repsonse message, populates both of the elements, and the server on the receiver side does likewise -- preferring streamPartitionMsgOffset. All callers into the protocol module have been modified to NOT set the offset field. Instead, only set the streamPartitionMsgOffset field. The 'offset' value will be derived from streamPartitionMsgOffset. Added a test to make sure that the controller always generates both elements. Such a test was not possible in the server side at this time, so verified manually. Manually ran LLCClusterIntergrationTest by disabling populating `streamPartitionMsgOffset` on the server side (old server/new controller) and on the controller respons side (new server/old controller) Issue #5359 ## Description Add a description of your PR here. A good description should include pointers to an issue or design document, etc. ## Upgrade Notes Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion) * [ ] Yes (Please label as **backward-incompat**, and complete the section below on Release Notes) Does this PR fix a zero-downtime upgrade introduced earlier? * [ ] Yes (Please label this as **backward-incompat**, and complete the section below on Release Notes) Does this PR otherwise need attention when creating release notes? Things to consider: - New configuration options - Deprecation of configurations - Signature changes to public methods/interfaces - New plugins added or old plugins removed * [ ] Yes (Please label this PR as **release-notes** and complete the section on Release Notes) ## Release Notes If you have tagged this as either backward-incompat or release-notes, you MUST add text here that you would like to see appear in release notes of the next release. If you have a series of commits adding or enabling a feature, then add this section only in final commit that marks the feature completed. Refer to earlier release notes to see examples of text ## Documentation If you have introduced a new feature or configuration, please add it to the documentation as well. See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document 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. 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
[GitHub] [incubator-pinot] jamesyfshao commented on pull request #5394: add callback interface for upsert component
jamesyfshao commented on pull request #5394: URL: https://github.com/apache/incubator-pinot/pull/5394#issuecomment-637721377 > Hey James, the last we discussed, I thought the only callback needed was after the realtime row was indexed. > > Can you add to your design doc the following information > > 1. At what points do you need callbacks from Pinot > 2. What does the callback do? > > For example: > Callbacks Needed: > > 1. onRowIndexed(): This callback needs to happen each time a row ingested from a realtime stream-partition is successfully indexed. upon receiving this callback, the pinot-upsert pushes an event into Kafka (see xxx of design doc). Arguments to this callback should be: >a. Offset of the row >b. Table name >c. Segment name >d. : whatever... > 2. onSegmentLoad(): This callback needs to happen each time an offline segment is loaded successfully. ... etc. > > I know this is probably spread across your design doc, but to have it concisely in one place will help us review. > > thanks a lot, and sorry for the delay in review. I had a hard deadline of May 31 on an internal project that I have been working on. thanks for reviewing, I have added a session [here](https://docs.google.com/document/d/1SFFir7ByxCff-aVYxQeTHpNhPXeP5q7P4g_6O2iNGgU/edit#heading=h.5g392vjhtywt). Also all the current callback are contained in this [package](https://github.com/jamesyfshao/pinot/tree/ecbef927507edd4239682aab3b0ef0f65a351a78/pinot-core/src/main/java/org/apache/pinot/core/data/manager/callback). If you have time to apply the patch to our local pinot repo it will be more straight forward and the method itself is also documnted 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. 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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5444: Enhance and simplify the filtering
Jackie-Jiang commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r434066870
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##
@@ -18,112 +18,52 @@
*/
package org.apache.pinot.core.operator.dociditerators;
-import java.util.Arrays;
import org.apache.pinot.core.common.BlockDocIdIterator;
import org.apache.pinot.core.common.Constants;
-// TODO: Optimize this
public final class AndDocIdIterator implements BlockDocIdIterator {
Review comment:
It is not thread safety (the whole filtering layer does not involve
thread safety because it is happening under one segment which is always
processed by one thread). Also, I don't think multiple threads using one
iterator make sense..
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5444: Enhance and simplify the filtering
Jackie-Jiang commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r434064515
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##
@@ -18,112 +18,52 @@
*/
package org.apache.pinot.core.operator.dociditerators;
-import java.util.Arrays;
import org.apache.pinot.core.common.BlockDocIdIterator;
import org.apache.pinot.core.common.Constants;
-// TODO: Optimize this
public final class AndDocIdIterator implements BlockDocIdIterator {
- public final BlockDocIdIterator[] docIdIterators;
- public ScanBasedDocIdIterator[] scanBasedDocIdIterators;
- public final int[] docIdPointers;
- public boolean reachedEnd = false;
- public int currentDocId = -1;
- int currentMax = -1;
- private boolean hasScanBasedIterators;
+ public final BlockDocIdIterator[] _docIdIterators;
- public AndDocIdIterator(BlockDocIdIterator[] blockDocIdIterators) {
-int numIndexBasedIterators = 0;
-int numScanBasedIterators = 0;
-for (int i = 0; i < blockDocIdIterators.length; i++) {
- if (blockDocIdIterators[i] instanceof IndexBasedDocIdIterator) {
-numIndexBasedIterators = numIndexBasedIterators + 1;
- } else if (blockDocIdIterators[i] instanceof ScanBasedDocIdIterator) {
-numScanBasedIterators = numScanBasedIterators + 1;
- }
-}
-// if we have at least one index based then do intersection based on index
based only, and then
-// check if matching docs apply on scan based iterator
-if (numIndexBasedIterators > 0 && numScanBasedIterators > 0) {
- hasScanBasedIterators = true;
- int nonScanIteratorsSize = blockDocIdIterators.length -
numScanBasedIterators;
- this.docIdIterators = new BlockDocIdIterator[nonScanIteratorsSize];
- this.scanBasedDocIdIterators = new
ScanBasedDocIdIterator[numScanBasedIterators];
- int nonScanBasedIndex = 0;
- int scanBasedIndex = 0;
- for (int i = 0; i < blockDocIdIterators.length; i++) {
-if (blockDocIdIterators[i] instanceof ScanBasedDocIdIterator) {
- this.scanBasedDocIdIterators[scanBasedIndex++] =
(ScanBasedDocIdIterator) blockDocIdIterators[i];
-} else {
- this.docIdIterators[nonScanBasedIndex++] = blockDocIdIterators[i];
-}
- }
-} else {
- hasScanBasedIterators = false;
- this.docIdIterators = blockDocIdIterators;
-}
-this.docIdPointers = new int[docIdIterators.length];
-Arrays.fill(docIdPointers, -1);
- }
+ private int _nextDocId = 0;
- @Override
- public int advance(int targetDocId) {
-if (currentDocId == Constants.EOF) {
- return currentDocId;
-}
-if (currentDocId >= targetDocId) {
- return currentDocId;
-}
-// next() method will always increment currentMax by 1.
-currentMax = targetDocId - 1;
-return next();
+ public AndDocIdIterator(BlockDocIdIterator[] docIdIterators) {
+_docIdIterators = docIdIterators;
}
@Override
public int next() {
-if (currentDocId == Constants.EOF) {
- return currentDocId;
-}
-currentMax = currentMax + 1;
-// always increment the pointer to current max, when this is called first
time, every one will
-// be set to start of posting list.
-for (int i = 0; i < docIdIterators.length; i++) {
- docIdPointers[i] = docIdIterators[i].advance(currentMax);
- if (docIdPointers[i] == Constants.EOF) {
-reachedEnd = true;
-currentMax = Constants.EOF;
-break;
- }
- if (docIdPointers[i] > currentMax) {
-currentMax = docIdPointers[i];
-if (i > 0) {
- // we need to advance all pointer since we found a new max
- i = -1;
-}
+int maxDocId = _nextDocId;
Review comment:
I prefer `AndDocIdIterator` because it performs an AND (intersection)
operation on all the iterators, and also it is the `BlockDocIdIterator` for
`AndDocIdSet`. Will add some javadoc showing the relation between them.
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.
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
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5444: Enhance and simplify the filtering
Jackie-Jiang commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r434063565
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ArrayBasedDocIdIterator.java
##
@@ -36,37 +35,15 @@ public ArrayBasedDocIdIterator(int[] docIds, int
searchableLength) {
@Override
public int next() {
-if (_currentDocId == Constants.EOF) {
- return Constants.EOF;
-}
-if (++_currentIndex == _searchableLength) {
- _currentDocId = Constants.EOF;
+if (_nextIndex < _searchableLength) {
+ return _docIds[_nextIndex++];
Review comment:
Caller should ensure `_searchableLength <= _docIds.length`. If caller
passes in wrong parameters, it is okay to through IndexOutOfBoundException.
Similarly, we don't perform null check everywhere for simplicity and
performance reason.
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.
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
[GitHub] [incubator-pinot] fx19880617 merged pull request #5480: Adding more information into jar manifest
fx19880617 merged pull request #5480: URL: https://github.com/apache/incubator-pinot/pull/5480 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. 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
[incubator-pinot] branch master updated (ed26e85 -> aae985f)
This is an automated email from the ASF dual-hosted git repository. xiangfu pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git. from ed26e85 [TE] fix document for adding new application (#5473) add aae985f Adding more information into jar manifest (#5480) No new revisions were added by this update. Summary of changes: pom.xml | 2 ++ 1 file changed, 2 insertions(+) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5478: Add a new table config field for peer segment download.
mcvsubbu commented on a change in pull request #5478:
URL: https://github.com/apache/incubator-pinot/pull/5478#discussion_r433971348
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##
@@ -201,4 +207,16 @@ public static void validate(TableConfig tableConfig) {
}
}
}
+
+ private static void validateValidationConfig(TableConfig tableConfig) {
+SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
+if (validationConfig != null) {
+ String peerSegmentDownloadScheme =
validationConfig.getPeerSegmentDownloadScheme();
+ if (peerSegmentDownloadScheme != null) {
+if (!"http".equals(peerSegmentDownloadScheme) &&
!"https".equals(peerSegmentDownloadScheme)) {
+ throw new IllegalStateException("peerSegmentDownloadScheme: " +
peerSegmentDownloadScheme + " is not http nor https" );
Review comment:
```suggestion
throw new IllegalStateException("Invalid value '" +
peerSegmentDownloadScheme + "' for peerSegmentDownloadScheme. Must be one of
http nor https" );
```
##
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java
##
@@ -201,4 +207,16 @@ public static void validate(TableConfig tableConfig) {
}
}
}
+
+ private static void validateValidationConfig(TableConfig tableConfig) {
+SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
+if (validationConfig != null) {
+ String peerSegmentDownloadScheme =
validationConfig.getPeerSegmentDownloadScheme();
+ if (peerSegmentDownloadScheme != null) {
+if (!"http".equals(peerSegmentDownloadScheme) &&
!"https".equals(peerSegmentDownloadScheme)) {
Review comment:
```suggestion
if (!"http".equalsIgnoreCase(peerSegmentDownloadScheme) &&
!"https".equalsIgnoreCase(peerSegmentDownloadScheme)) {
```
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.
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
[GitHub] [incubator-pinot] haibow opened a new issue #5485: Remove LinkedIn config from pom
haibow opened a new issue #5485: URL: https://github.com/apache/incubator-pinot/issues/5485 In #5190, LinkedIn deployment configs were added to the pom file. This caused Apache release to fail, and I had to remove the following lines: https://github.com/apache/incubator-pinot/blob/ed26e8589fe5f91d2876d417aebf23575010cc76/pom.xml#L100-L106 We should try to remove LinkedIn specific configs from the repo. One alternative solution is to configure distributionManagement in command line: https://stackoverflow.com/questions/12435283/adding-a-maven-distribution-repository-on-the-command-line Adding this task to remove the config above without breaking LinkedIn's nightly build. 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. 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
svn commit: r39878 - /dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/
Author: haibow Date: Tue Jun 2 06:49:10 2020 New Revision: 39878 Log: Update apache-pinot-incubating-0.4.0-rc2 Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/ dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz (with props) dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.asc dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.sha512 dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz (with props) dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz.asc dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz.sha512 Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz == Binary file - no diff available. Propchange: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz -- svn:mime-type = application/octet-stream Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.asc == --- dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.asc (added) +++ dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.asc Tue Jun 2 06:49:10 2020 @@ -0,0 +1,16 @@ +-BEGIN PGP SIGNATURE- + +iQIzBAABCgAdFiEEyH5xT96Wd9MlHHp5bMFppvwZxHAFAl7V8TIACgkQbMFppvwZ +xHCCOxAAgcS8toKtgiR1dxrMoVB4DYt2BF0jPsONoxRb2yiotpncfJPw9V++85f2 +eMkFE/4crqXhnvvjnhZpbOqA4/mds0fwwTMz2F/nLhavB70NZqNbx5yYo4eqXJj4 +NEn/go744cDzq9gxT/0OIKCffEVYJqyQag/Ww0yb8KRkZO1wth15k2p/xuum4Qet +4ON5lfa6FaIy/pGrdhuT353GnNgMjCwbQh7By82Ib+hyhMcoYvXSi/AnpmoeWc1n +6SC4561NwJlN6YmPVoJkXUaGaaPriXgXdHpy2llCYULt4Q/HE/e7cgV1Hy/BTydG +GeKWdg8pzqkcK0h/JFJdM49RbneW8wJbQOLTl/XcgMugqkUCm3to5YR0cncVNpCH +DHDLUSzkKtdhqNCLeG+0tCBJ5nKY9kz0dlYW5yGWXiRoYyTeMyNb/KPcTzL3a2fv +r7AiQ7/mnB0x5zYlc4Dmed2+CF+HYEVivvyzOKDS+N8fhbjIUcidsMa3m5ac9gi8 +12uR8WyCd5n3wrwQl3a76KSkO9Q+VISbPZZ3ovN6ymOOuVrK03lWzcSwWA8g0XNl +AJJMQb4R1QTucU2ApwXglfSUD9r5y9unUU+XyGgfRNxrhOc6klnWhyWhMYo09Vdq +GJgKLgHpUiMI65PHSpYVF3AORE68ma0YKx/NZZep41Gr3mn76ns= +=fa1X +-END PGP SIGNATURE- Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.sha512 == --- dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.sha512 (added) +++ dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-bin.tar.gz.sha512 Tue Jun 2 06:49:10 2020 @@ -0,0 +1 @@ +155494c13501bad1adb626cf204079d024e72b1ce700ab55c8de2e7f4a7166b77f3715b9af9ff2e077681ca11e4cc87987433cd56572226688999dc9e413 \ No newline at end of file Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz == Binary file - no diff available. Propchange: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz -- svn:mime-type = application/octet-stream Added: dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz.asc == --- dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz.asc (added) +++ dev/incubator/pinot/apache-pinot-incubating-0.4.0-rc2/apache-pinot-incubating-0.4.0-src.tar.gz.asc Tue Jun 2 06:49:10 2020 @@ -0,0 +1,16 @@ +-BEGIN PGP SIGNATURE- + +iQIzBAABCgAdFiEEyH5xT96Wd9MlHHp5bMFppvwZxHAFAl7V8TEACgkQbMFppvwZ +xHACDBAAjCsrfhSmaw1fKZ0ot5SE1FHVP00A6Sa2y5csVW7XpAklaefFvzR8LGLZ ++OpMXH7/BJn/pf054UnHFCntWQBPyZcZfNyVSxxT6h39F1JDE8Rbe4y7VAsWwn8a +YaCBbW/U9gp69OTRHyKHerTbDe89KczZD02xsgxiW6N4lxvyGEcE9DBtzJwhuc35 +pp8z3s2wYFv45j4myuyU7KE3NORB73dqsqkV0Y7nccilLHGR96GNEFeBaGRnUMip +T2IfXge+e6QSfKhii/wYKOs2+dOPcdflV9rMiLqsL3BH6ajAPGnyc2+poAa1eX0e +wf+75yMg8nfenyFnaE1iQo5cnbd2354fowEMprJj5M44YcMAq2U6xPbbCBWqGaqO +rotrIRAy/K6yxovJQarGtgyqpp2pTXP0k4MlcwdzhZfEmp8pZrOpwCCaGQ0/fn/U +Qcr//wTsqA576Ha1eE3YoLZN+7MDLH9H6EpOpz0vMJrsB3XQbz0UfTcCnD5eyJ98 +qr9If90rJXbkNdttr1ODbz6TqHnwjAM8w0N1Y8WPzhyOn5/+U8Y3MHmskMEvRw2w +tKan6DmogpJHz4NnklDzcV+mti1W8IBs+/kyUItopjf++xmhMDcqCZPZWTs6/omQ +ond7Vh1tjfG6Lzdj7lA5NE+8WdDWC7pICiwz2SUdgF/UBmStaLI= +=uGOJ +-END PGP SIGNATURE- Added:
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5444: Enhance and simplify the filtering
chenboat commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r433647278
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##
@@ -18,112 +18,52 @@
*/
package org.apache.pinot.core.operator.dociditerators;
-import java.util.Arrays;
import org.apache.pinot.core.common.BlockDocIdIterator;
import org.apache.pinot.core.common.Constants;
-// TODO: Optimize this
public final class AndDocIdIterator implements BlockDocIdIterator {
Review comment:
1.javadoc?
2. comments about thread safety. is it safe/advisable to use this iterator
in more than one thread?
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.
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
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5444: Enhance and simplify the filtering
chenboat commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r433642778
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ArrayBasedDocIdIterator.java
##
@@ -36,37 +35,15 @@ public ArrayBasedDocIdIterator(int[] docIds, int
searchableLength) {
@Override
public int next() {
-if (_currentDocId == Constants.EOF) {
- return Constants.EOF;
-}
-if (++_currentIndex == _searchableLength) {
- _currentDocId = Constants.EOF;
+if (_nextIndex < _searchableLength) {
+ return _docIds[_nextIndex++];
Review comment:
what happens if _nextIndex >= docIds.length? should we have a array
length check?
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.
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
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5444: Enhance and simplify the filtering
chenboat commented on a change in pull request #5444:
URL: https://github.com/apache/incubator-pinot/pull/5444#discussion_r433641032
##
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##
@@ -18,112 +18,52 @@
*/
package org.apache.pinot.core.operator.dociditerators;
-import java.util.Arrays;
import org.apache.pinot.core.common.BlockDocIdIterator;
import org.apache.pinot.core.common.Constants;
-// TODO: Optimize this
public final class AndDocIdIterator implements BlockDocIdIterator {
- public final BlockDocIdIterator[] docIdIterators;
- public ScanBasedDocIdIterator[] scanBasedDocIdIterators;
- public final int[] docIdPointers;
- public boolean reachedEnd = false;
- public int currentDocId = -1;
- int currentMax = -1;
- private boolean hasScanBasedIterators;
+ public final BlockDocIdIterator[] _docIdIterators;
- public AndDocIdIterator(BlockDocIdIterator[] blockDocIdIterators) {
-int numIndexBasedIterators = 0;
-int numScanBasedIterators = 0;
-for (int i = 0; i < blockDocIdIterators.length; i++) {
- if (blockDocIdIterators[i] instanceof IndexBasedDocIdIterator) {
-numIndexBasedIterators = numIndexBasedIterators + 1;
- } else if (blockDocIdIterators[i] instanceof ScanBasedDocIdIterator) {
-numScanBasedIterators = numScanBasedIterators + 1;
- }
-}
-// if we have at least one index based then do intersection based on index
based only, and then
-// check if matching docs apply on scan based iterator
-if (numIndexBasedIterators > 0 && numScanBasedIterators > 0) {
- hasScanBasedIterators = true;
- int nonScanIteratorsSize = blockDocIdIterators.length -
numScanBasedIterators;
- this.docIdIterators = new BlockDocIdIterator[nonScanIteratorsSize];
- this.scanBasedDocIdIterators = new
ScanBasedDocIdIterator[numScanBasedIterators];
- int nonScanBasedIndex = 0;
- int scanBasedIndex = 0;
- for (int i = 0; i < blockDocIdIterators.length; i++) {
-if (blockDocIdIterators[i] instanceof ScanBasedDocIdIterator) {
- this.scanBasedDocIdIterators[scanBasedIndex++] =
(ScanBasedDocIdIterator) blockDocIdIterators[i];
-} else {
- this.docIdIterators[nonScanBasedIndex++] = blockDocIdIterators[i];
-}
- }
-} else {
- hasScanBasedIterators = false;
- this.docIdIterators = blockDocIdIterators;
-}
-this.docIdPointers = new int[docIdIterators.length];
-Arrays.fill(docIdPointers, -1);
- }
+ private int _nextDocId = 0;
- @Override
- public int advance(int targetDocId) {
-if (currentDocId == Constants.EOF) {
- return currentDocId;
-}
-if (currentDocId >= targetDocId) {
- return currentDocId;
-}
-// next() method will always increment currentMax by 1.
-currentMax = targetDocId - 1;
-return next();
+ public AndDocIdIterator(BlockDocIdIterator[] docIdIterators) {
+_docIdIterators = docIdIterators;
}
@Override
public int next() {
-if (currentDocId == Constants.EOF) {
- return currentDocId;
-}
-currentMax = currentMax + 1;
-// always increment the pointer to current max, when this is called first
time, every one will
-// be set to start of posting list.
-for (int i = 0; i < docIdIterators.length; i++) {
- docIdPointers[i] = docIdIterators[i].advance(currentMax);
- if (docIdPointers[i] == Constants.EOF) {
-reachedEnd = true;
-currentMax = Constants.EOF;
-break;
- }
- if (docIdPointers[i] > currentMax) {
-currentMax = docIdPointers[i];
-if (i > 0) {
- // we need to advance all pointer since we found a new max
- i = -1;
-}
+int maxDocId = _nextDocId;
Review comment:
i think a better name could be candidateDocId because the while loop()
is searching for the next doc Id exists in all iterators. maxDocId to many ppl
mean the maximum doc id in the iterator.
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.
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
