[GitHub] [pinot] walterddr commented on a diff in pull request #10154: [multistage] make mailboxID JSON serialized
walterddr commented on code in PR #10154:
URL: https://github.com/apache/pinot/pull/10154#discussion_r1082874473
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/JsonMailboxIdentifier.java:
##
@@ -0,0 +1,132 @@
+/**
+ * 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.mailbox;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Objects;
+
+
+public class JsonMailboxIdentifier implements MailboxIdentifier {
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private final String _jobId;
+ private final String _from;
+ private final String _to;
+
+ private final ServerAddress _fromAddress;
+ private final ServerAddress _toAddress;
+
+ @JsonCreator
+ public JsonMailboxIdentifier(
+ @JsonProperty(value = "jobId") String jobId,
Review Comment:
yes discussed offline and here is the summaryL:
- making it a full blown JSON next will probably cause another backward
compatibility issue but will make it much more flexible
- refactoring it as a jobID object with variable member field will not cause
backward compatibility issues but it doesn't need to stack on this 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
[GitHub] [pinot] walterddr merged pull request #10154: [multistage] make mailboxID JSON serialized
walterddr merged PR #10154: URL: https://github.com/apache/pinot/pull/10154 -- 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
[GitHub] [pinot] agavra commented on a diff in pull request #10154: [multistage] make mailboxID JSON serialized
agavra commented on code in PR #10154:
URL: https://github.com/apache/pinot/pull/10154#discussion_r1082866431
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/JsonMailboxIdentifier.java:
##
@@ -0,0 +1,132 @@
+/**
+ * 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.mailbox;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Objects;
+
+
+public class JsonMailboxIdentifier implements MailboxIdentifier {
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private final String _jobId;
+ private final String _from;
+ private final String _to;
+
+ private final ServerAddress _fromAddress;
+ private final ServerAddress _toAddress;
+
+ @JsonCreator
+ public JsonMailboxIdentifier(
+ @JsonProperty(value = "jobId") String jobId,
Review Comment:
talked offline, if we want to add more we can add them as top level concepts
or (if they don't need to be deserialized) we can just encode them into the
`jobId` 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.
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
[GitHub] [pinot] agavra commented on a diff in pull request #10154: [multistage] make mailboxID JSON serialized
agavra commented on code in PR #10154:
URL: https://github.com/apache/pinot/pull/10154#discussion_r1082859240
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/JsonMailboxIdentifier.java:
##
@@ -0,0 +1,132 @@
+/**
+ * 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.mailbox;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Objects;
+
+
+public class JsonMailboxIdentifier implements MailboxIdentifier {
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private final String _jobId;
+ private final String _from;
+ private final String _to;
+
+ private final ServerAddress _fromAddress;
+ private final ServerAddress _toAddress;
+
+ @JsonCreator
+ public JsonMailboxIdentifier(
+ @JsonProperty(value = "jobId") String jobId,
Review Comment:
yeah, I think for jobId it might make most sense to actually make that a
full blown JSON object WDYT?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10154: [multistage] make mailboxID JSON serialized
walterddr commented on code in PR #10154:
URL: https://github.com/apache/pinot/pull/10154#discussion_r1082852866
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/JsonMailboxIdentifier.java:
##
@@ -0,0 +1,132 @@
+/**
+ * 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.mailbox;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.util.Objects;
+
+
+public class JsonMailboxIdentifier implements MailboxIdentifier {
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private final String _jobId;
+ private final String _from;
+ private final String _to;
+
+ private final ServerAddress _fromAddress;
+ private final ServerAddress _toAddress;
+
+ @JsonCreator
+ public JsonMailboxIdentifier(
+ @JsonProperty(value = "jobId") String jobId,
Review Comment:
IIUC jobID is also a combo string of requestID + stageID
is it possible to also change this to a job object similar to serverAddress?
later on with support of LogicalSpool we might actually need to expand the
support to requestID + senderStageID + receiverStageID
--
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
[GitHub] [pinot] GSharayu commented on a diff in pull request #10132: add API to get progress of subtasks with given state
GSharayu commented on code in PR #10132:
URL: https://github.com/apache/pinot/pull/10132#discussion_r1082845704
##
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java:
##
@@ -419,7 +423,7 @@ public Map getSubtaskConfigs(
@GET
@Path("/tasks/subtask/{taskName}/progress")
@Produces(MediaType.APPLICATION_JSON)
- @ApiOperation("Get progress of specified sub tasks for the given task
tracked by worker in memory")
+ @ApiOperation("Get progress of specified sub tasks for the given task
tracked by minion worker in memory")
Review Comment:
+1 on using the task names
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10089: Reload conditional skip
klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1082844023
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##
@@ -403,6 +417,16 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
}
+ public boolean canReuseExistingDirectoryForReload(SegmentZKMetadata
segmentZKMetadata,
+ String currentSegmentTier, SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
+ Schema schema)
+ throws Exception {
+SegmentDirectoryLoader segmentDirectoryLoader =
+
SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader(indexLoadingConfig.getSegmentDirectoryLoader());
+return
!segmentDirectoryLoader.needsTierMigration(segmentZKMetadata.getTier(),
currentSegmentTier)
+&& !ImmutableSegmentLoader.needPreprocess(segmentDirectory,
indexLoadingConfig, schema);
Review Comment:
looks like we can make this method private
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10089: Reload conditional skip
klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1082834046
##
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoader.java:
##
@@ -42,4 +41,13 @@ SegmentDirectory load(URI indexDir,
SegmentDirectoryLoaderContext segmentDirecto
default void delete(SegmentDirectoryLoaderContext
segmentDirectoryLoaderContext)
throws Exception {
}
+
+ /**
+ * Based on the zkMetadata's and current segment tier, checks whether or not
tier migration is needed
+ * @param zkTier segment's ZKMetadata's tier
+ * @param currentTier Current segment tier
+ */
+ default boolean needsTierMigration(String zkTier, String currentTier) {
Review Comment:
nit: rename zkTier to targetTier to be more generic
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10089: Reload conditional skip
klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1082833141
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##
@@ -403,6 +417,16 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
}
+ public boolean canReuseExistingDirectoryForReload(SegmentZKMetadata
segmentZKMetadata,
+ String currentSegmentTier, SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
+ Schema schema)
+ throws Exception {
+SegmentDirectoryLoader segmentDirectoryLoader =
+
SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader(indexLoadingConfig.getSegmentDirectoryLoader());
+return
!segmentDirectoryLoader.needsTierMigration(segmentZKMetadata.getTier(),
currentSegmentTier)
+&& !ImmutableSegmentLoader.needPreprocess(segmentDirectory,
indexLoadingConfig, schema);
Review Comment:
yes, it also depends on if the segdir loader can migrate tier or not, and it
seems more reasonable to have loader to check this, e.g. both
TierBasedSegmentDirectoryLoader and DefaultSegmentDirectoryLoader returns
SegmentLocalFSDirectory and only the former needs to do tier migration.
--
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
[GitHub] [pinot] KKcorps commented on pull request #10155: Make a system property to call system.exit() for LaunchDataIngestionJobCommand
KKcorps commented on PR #10155: URL: https://github.com/apache/pinot/pull/10155#issuecomment-1398468246 This System.exit call was added in the first place to fail the spark job. Otherwise it just returned success even in the case of failure -- 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
[GitHub] [pinot] ankitsultana commented on issue #10129: [multistage] Projection Not Pushed Down for Agg Queries with No Grouping Sets
ankitsultana commented on issue #10129: URL: https://github.com/apache/pinot/issues/10129#issuecomment-1398417863 @walterddr : Can we consider using `RelFieldTrimmer`? I raised this: https://github.com/apache/pinot/pull/10156 It seems to work and I am testing it out on our clusters. The PR is not final as I am also reading more about this feature. There are some caveats called out in the code also regarding 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. 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
[GitHub] [pinot] ankitsultana opened a new pull request, #10156: [multistage] Trim Unused Fields Before Optimizer Runs
ankitsultana opened a new pull request, #10156: URL: https://github.com/apache/pinot/pull/10156 This seems to fix projection pushdown issues. PR is not really final since I am also reading more about this. There's a method to enable this via `SqlRelConverterConfig.config().withTrimUnusedFields(true)`, but the behavior of `SqlRelConverterConfig::convertQuery` doesn't really change and fields are not trimmed. I think it only trims the fields for prepared queries. cc: @walterddr -- 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
[GitHub] [pinot] Ferix9288 commented on pull request #10139: JMX to Prom Exporter - More Inclusive Table Regex
Ferix9288 commented on PR #10139: URL: https://github.com/apache/pinot/pull/10139#issuecomment-1398240019 @Jackie-Jiang changed as you described and confirmed working with a custom image in our `dev` environment -- 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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10089: Reload conditional skip
saurabhd336 commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1082367099
##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##
@@ -207,11 +207,7 @@ private boolean needProcessStarTrees() {
List starTreeMetadataList =
_segmentMetadata.getStarTreeV2MetadataList();
// There are existing star-trees, but if they match the builder configs
exactly,
// then there is no need to generate the star-trees
-if (starTreeMetadataList != null && !StarTreeBuilderUtils
-.shouldRemoveExistingStarTrees(starTreeBuilderConfigs,
starTreeMetadataList)) {
- return false;
-}
-return !starTreeBuilderConfigs.isEmpty();
+return
StarTreeBuilderUtils.existingStarTreesNeedChange(starTreeBuilderConfigs,
starTreeMetadataList);
Review Comment:
Ack. This will work.
--
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
[GitHub] [pinot] navina commented on pull request #10136: Decouple dependency on using peer segment download scheme for doing metadata pushes to controller
navina commented on PR #10136: URL: https://github.com/apache/pinot/pull/10136#issuecomment-1398019400 > Please check the test failure. I think it is related to the current change Yes will fix it. Ty -- 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
[GitHub] [pinot] codecov-commenter commented on pull request #10155: Make a system property to call system.exit() for LaunchDataIngestionJobCommand
codecov-commenter commented on PR #10155: URL: https://github.com/apache/pinot/pull/10155#issuecomment-1398016275 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10155](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (ed0a112) into [master](https://codecov.io/gh/apache/pinot/commit/4d7da0abab9563d95d139e11d054da2f0f5d45d8?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (4d7da0a) will **decrease** coverage by `21.19%`. > The diff coverage is `n/a`. ```diff @@ Coverage Diff @@ ## master #10155 +/- ## = - Coverage 34.84% 13.65% -21.19% + Complexity 200 176 -24 = Files 2006 1951 -55 Lines108718 106232 -2486 Branches 1650116207 -294 = - Hits 3788014510-23370 - Misses6756690583+23017 + Partials 3272 1139 -2133 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `?` | | | integration2 | `?` | | | unittests2 | `13.65% <ø> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/common/CustomObject.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vQ3VzdG9tT2JqZWN0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/common/auth/AuthConfig.java](https://codecov.io/gh/apache/pinot/pull/10155?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXV0aC9BdXRoQ29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | |
[GitHub] [pinot] xiangfu0 opened a new pull request, #10155: Make a system property to call system.exit() for LaunchDataIngestionJobCommand
xiangfu0 opened a new pull request, #10155: URL: https://github.com/apache/pinot/pull/10155 Use a system property to call system.exit() for LaunchDataIngestionJobCommand. The reason is SparkJob may fail due to this system.exit() call. -- 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
[GitHub] [pinot] codecov-commenter commented on pull request #10154: [multistage] make mailboxID JSON serialized
codecov-commenter commented on PR #10154: URL: https://github.com/apache/pinot/pull/10154#issuecomment-1397915490 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10154](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (99d6119) into [master](https://codecov.io/gh/apache/pinot/commit/4d7da0abab9563d95d139e11d054da2f0f5d45d8?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (4d7da0a) will **decrease** coverage by `10.16%`. > The diff coverage is `0.00%`. ```diff @@ Coverage Diff @@ ## master #10154 +/- ## = - Coverage 34.84% 24.68% -10.16% + Complexity 200 44 -156 = Files 2006 1995 -11 Lines108718 108388 -330 Branches 1650116467 -34 = - Hits 3788026754-11126 - Misses6756678830+11264 + Partials 3272 2804 -468 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `24.68% <0.00%> (+0.17%)` | :arrow_up: | | integration2 | `?` | | | unittests2 | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...he/pinot/query/mailbox/InMemorySendingMailbox.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9Jbk1lbW9yeVNlbmRpbmdNYWlsYm94LmphdmE=) | `0.00% <0.00%> (ø)` | | | [...che/pinot/query/mailbox/JsonMailboxIdentifier.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9Kc29uTWFpbGJveElkZW50aWZpZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | | | [.../org/apache/pinot/query/mailbox/ServerAddress.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9TZXJ2ZXJBZGRyZXNzLmphdmE=) | `0.00% <0.00%> (ø)` | | | [...ain/java/org/apache/pinot/query/mailbox/Utils.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9VdGlscy5qYXZh) | `0.00% <0.00%> (ø)` | | | [.../mailbox/channel/MailboxContentStreamObserver.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvbWFpbGJveC9jaGFubmVsL01haWxib3hDb250ZW50U3RyZWFtT2JzZXJ2ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | | | [...query/runtime/operator/MailboxReceiveOperator.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94UmVjZWl2ZU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | | | [...ot/query/runtime/operator/MailboxSendOperator.java](https://codecov.io/gh/apache/pinot/pull/10154?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | | |
[GitHub] [pinot] agavra opened a new pull request, #10154: [multistage] make mailboxID JSON serialized
agavra opened a new pull request, #10154:
URL: https://github.com/apache/pinot/pull/10154
Minor PR to change the serialization of mailbox identifier to a JSON format.
For example:
```
{"jobId":"-7324288832996730535_2","from":"localhost:52683","to":"localhost:52683"}
```
This is the first PR in a series to introduce virtual server. It makes it
easier for me to change the `ServerAddress` to also include a partition
identifier.
**NOTE**: I chose to encode the server address in `:`-delimited instead of
JSON to make it easier to read and send fewer bytes on the wire, but it does
make them less flexible going forward (you can only add things at the end).
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10153: allow editing pools in instance details ui
codecov-commenter commented on PR #10153: URL: https://github.com/apache/pinot/pull/10153#issuecomment-139776 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10153](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (70c2e2c) into [master](https://codecov.io/gh/apache/pinot/commit/c9ef76fb0471175c5d7a412dca46d10a2e2f2e2a?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (c9ef76f) will **decrease** coverage by `56.71%`. > The diff coverage is `n/a`. ```diff @@ Coverage Diff @@ ## master #10153 +/- ## = - Coverage 70.35% 13.65% -56.71% + Complexity 5134 176 -4958 = Files 2000 1951 -49 Lines108454 106232 - Branches 1648116207 -274 = - Hits 7630214503-61799 - Misses2681290583+63771 + Partials 5340 1146 -4194 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `?` | | | integration2 | `?` | | | unittests1 | `?` | | | unittests2 | `13.65% <ø> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/spi/utils/LoopUtils.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTG9vcFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/10153?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | |
[GitHub] [pinot] jadami10 opened a new pull request, #10153: allow editing pools in instance details ui
jadami10 opened a new pull request, #10153: URL: https://github.com/apache/pinot/pull/10153 This is a simple ui feature + bugfix. Today there's two issues: 1) there's no way from the UI to easily create or update pools from the instance details config page 2) if you are using pools, and you accidentally use the `Edit Config` modal, you will delete your pools because they get reset to null (this should probably be fixed in the java API) This just puts the pools field in the `instancePutObj` var so it's always included in the edit UI. https://user-images.githubusercontent.com/4760722/213609437-d6244e72-b045-48a4-b3f7-1bca568b5cdd.png;> I tested a few scenarios here: - going from null -> a correctly formatted pool map works - there's no validation that the pool name is one of the tags which I think is necessary, but the current API isn't doing that validation - adding pools works - removing pools works - setting it back to null also works - trying to use a non-map value like a string breaks with a 400 error which is likely good enough for 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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10151: Simplify UTF-8 comparison on null character
Jackie-Jiang commented on code in PR #10151:
URL: https://github.com/apache/pinot/pull/10151#discussion_r1082047825
##
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readerwriter/ValueReaderComparisonTest.java:
##
@@ -156,22 +156,27 @@ void testSuperiorPrefixes(ValueReader reader, int
numBytesPerValue, String... st
void testConsistent(ValueReader reader, int numBytesPerValue, int
numValuesToCompare) {
for (int i = 0; i < numValuesToCompare; i++) {
byte[] bytes = new
byte[ThreadLocalRandom.current().nextInt(numBytesPerValue * 2)];
-assertConsistentUtf8Comparison(reader, i, numBytesPerValue, new
String(bytes, StandardCharsets.UTF_8));
-for (int j = 0; j < 128; j++) {
+
+for (int j = 1; j < 128; j++) {
Review Comment:
Yeah, we skipped them in the test
--
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
[GitHub] [pinot] jadami10 commented on pull request #9994: [feature] Add a Tracker class to support aggregate-worst case consumption delay…
jadami10 commented on PR #9994: URL: https://github.com/apache/pinot/pull/9994#issuecomment-1397833536 I tried deploying this change to one of our clusters with #10101 or #10121. We currently track ingestion lag via our custom stream ingestion plugin. For the most part these match up. But I still see an issue where partitions that have infrequent events continue to report forever increasing lag. From the code comments it seems that shouldn't be the case, but I want to make sure before I dig into our plugin to see if something isn't implemented as expected. -- 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #10151: Simplify UTF-8 comparison on null character
Jackie-Jiang commented on PR #10151:
URL: https://github.com/apache/pinot/pull/10151#issuecomment-139782
With the current code, `length == bytes.length && length != 0 &&
bytes[length - 1] == 0` case is not handled (the new added
`ValueReaderComparisonTest` is flaky right now). To keep the behavior exactly
the same as before, we need to do the following check:
```
if (padded) {
if (length < bytes.length) {
return -1;
}
if (length == bytes.length) {
if (length == 0) {
return 0;
} else {
return dataBuffer.getByte(startOffset + length - 1) == 0 ? -1 :
0;
}
}
// length > bytes.length
// need to figure out whether the unpadded string is longer than the
parameter or not
if (bytes.length == 0) {
// just need nonzero first byte
return dataBuffer.getByte(startOffset) == 0 ? 0 : 1;
} else if (bytes[bytes.length - 1] == 0) {
return -1;
} else {
// check if the stored string continues beyond the length of the
parameter
return dataBuffer.getByte(startOffset + bytes.length) == 0 ? 0 : 1;
}
```
IMO it is too much and quite easy to make mistakes without adding much value
--
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
[GitHub] [pinot] sajjad-moradi commented on a diff in pull request #10121: Add a tracker for end-to-end consumption delay of events.
sajjad-moradi commented on code in PR #10121:
URL: https://github.com/apache/pinot/pull/10121#discussion_r1082035842
##
pinot-spi/src/main/java/org/apache/pinot/spi/stream/RowMetadata.java:
##
@@ -49,6 +49,17 @@ public interface RowMetadata {
*/
long getRecordIngestionTimeMs();
+ /**
+ * Returns the creation timestamp associated with the record. In cases where
the upstream ingestion pipeline is
+ * simple this timestamp matches the result of getRecordIngestionTimeMs();
+ *
+ * Expected to be used for stream-based sources.
+ *
+ * @return timestamp (epoch in milliseconds) when the row was initially
created and ingested upstream for the first
+ * time Long.MIN_VALUE if not available
+ */
+ long getRecordCreationTimeMs();
Review Comment:
Should we return the default value Long.MIN_VALUE to not break different
streams' implementations?
--
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
[GitHub] [pinot] sajjad-moradi commented on a diff in pull request #10121: Add a tracker for end-to-end consumption delay of events.
sajjad-moradi commented on code in PR #10121:
URL: https://github.com/apache/pinot/pull/10121#discussion_r1082029067
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -76,6 +76,15 @@
public class IngestionDelayTracker {
+ // Class to wrap supported timestamps collected for an ingested event
+ private static class IngestionTimestamps {
+IngestionTimestamps(long ingestionTimesMs, long creationTimeMs) {
+ _ingestionTimeMs = ingestionTimesMs;
+ _creationTimeMs = creationTimeMs;
Review Comment:
`_ingestionTimeMs` is basically the time when a message was published to the
last stream.
`_creationTimeMs` is the time when a message was published to the first
stream.
Should we rename them to something like `lastStreamPublishTimeMs` and
`firstStreamPublishTimeMs`? Or at least we need to have some comments to
clearly specify what each variable is.
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##
@@ -615,7 +615,8 @@ private boolean processStreamEvents(MessageBatch
messagesAndOffsets, long idlePi
}
} else if (!prematureExit) {
// Record Pinot ingestion delay as zero since we are up-to-date and no
new events
-
_realtimeTableDataManager.updateIngestionDelay(System.currentTimeMillis(),
_partitionGroupId);
+
_realtimeTableDataManager.updateIngestionDelay(System.currentTimeMillis(),
System.currentTimeMillis(),
Review Comment:
Refactor to `long now = System.currentTimeMillis(); ...`
##
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTrackerTest.java:
##
@@ -85,29 +85,37 @@ public void testRecordIngestionDelayWithNoAging() {
// Test we follow a single partition up and down
for (long i = 0; i <= maxTestDelay; i++) {
- ingestionDelayTracker.updateIngestionDelay(i, partition0);
+ ingestionDelayTracker.updateIngestionDelay(i, (i + 1), partition0);
Assert.assertEquals(ingestionDelayTracker.getPartitionIngestionDelayMs(partition0),
clock.millis() - i);
+
Assert.assertEquals(ingestionDelayTracker.getPartitionEndToEndIngestionDelayMs(partition0),
+ clock.millis() - (i + 1));
Review Comment:
Ditto. Please remove all other redundant parentheses.
##
pinot-spi/src/main/java/org/apache/pinot/spi/stream/RowMetadata.java:
##
@@ -49,6 +49,17 @@ public interface RowMetadata {
*/
long getRecordIngestionTimeMs();
+ /**
+ * Returns the creation timestamp associated with the record. In cases where
the upstream ingestion pipeline is
+ * simple this timestamp matches the result of getRecordIngestionTimeMs();
+ *
+ * Expected to be used for stream-based sources.
+ *
+ * @return timestamp (epoch in milliseconds) when the row was initially
created and ingested upstream for the first
+ * time Long.MIN_VALUE if not available
+ */
+ long getRecordCreationTimeMs();
Review Comment:
Shouldn't we return the default value Long.MIN_VALUE to not break different
streams' implementations?
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -76,6 +76,15 @@
public class IngestionDelayTracker {
+ // Class to wrap supported timestamps collected for an ingested event
+ private static class IngestionTimestamps {
+IngestionTimestamps(long ingestionTimesMs, long creationTimeMs) {
+ _ingestionTimeMs = ingestionTimesMs;
+ _creationTimeMs = creationTimeMs;
+}
+public final long _ingestionTimeMs;
+public final long _creationTimeMs;
Review Comment:
Do these need to be public?
##
pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTrackerTest.java:
##
@@ -85,29 +85,37 @@ public void testRecordIngestionDelayWithNoAging() {
// Test we follow a single partition up and down
for (long i = 0; i <= maxTestDelay; i++) {
- ingestionDelayTracker.updateIngestionDelay(i, partition0);
+ ingestionDelayTracker.updateIngestionDelay(i, (i + 1), partition0);
Review Comment:
Parentheses are not needed in `(i + 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
[GitHub] [pinot] Jackie-Jiang closed pull request #10144: Adding duration unit in trivy timeout
Jackie-Jiang closed pull request #10144: Adding duration unit in trivy timeout URL: https://github.com/apache/pinot/pull/10144 -- 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #10144: Adding duration unit in trivy timeout
Jackie-Jiang commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1397822649 This change is included in #10150 which also fixes the trivy failure -- 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
[GitHub] [pinot] Jackie-Jiang merged pull request #10150: Fix json-smart version
Jackie-Jiang merged PR #10150: URL: https://github.com/apache/pinot/pull/10150 -- 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
[GitHub] [pinot] mcvsubbu commented on issue #10147: Pauseless Consumption
mcvsubbu commented on issue #10147: URL: https://github.com/apache/pinot/issues/10147#issuecomment-1397819480 How many rows would the new consuming segment be setup to consume? (Currently that is decided when the prev segment is committed). -- 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #10139: JMX to Prom Exporter - More Inclusive Table Regex
Jackie-Jiang commented on PR #10139: URL: https://github.com/apache/pinot/pull/10139#issuecomment-1397807868 Whitespaces in table name is definitely not recommended, but I don't think we reject it right now. Actually it is also not recommended to have '-' in table name or column name because you have to quote the name in the query, or it won't compile because '-' will be parsed as `minus`. I'd suggest changing the regex to `(.*?)_(OFFLINE|REALTIME)` which supports all possible table names, but also limit the table type value to prevent unexpected behavior. -- 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
[GitHub] [pinot] Jackie-Jiang commented on issue #10147: Pauseless Consumption
Jackie-Jiang commented on issue #10147: URL: https://github.com/apache/pinot/issues/10147#issuecomment-1397794076 Have you considered using the regular state transition to achieve this? Essentially we can create the new consuming segment in IdealState when the commit starts. Nothing needs to be changed in the routing or state transition handling. If the commit end didn't arrive or failed, we can remove the new consuming segment from the IdealState. I feel this approach is more natural and easier to implement -- 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
[GitHub] [pinot] Jackie-Jiang commented on pull request #10151: Simplify UTF-8 comparison on null character
Jackie-Jiang commented on PR #10151: URL: https://github.com/apache/pinot/pull/10151#issuecomment-1397787608 > Does `LoaderTest.testPadding` pass with this change? I'm all for simplification (assuming whatever the motivation for adding `LoaderTest.testPadding` was no longer matters) but this isn't going to move the needle much performance wise. This test is removed in #10087 where we removed the non-zero padding support. That test is added mainly for testing the behavior of legacy '%' padding, which is much more common than null. -- 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
[GitHub] [pinot] navina opened a new issue, #10152: Support consumer de-aggregation in Kinesis
navina opened a new issue, #10152: URL: https://github.com/apache/pinot/issues/10152 Kinesis supports producing aggregated (aka batched) record. Thus, kinesis consumer also has support for de-aggregating the records. Refer - https://docs.aws.amazon.com/streams/latest/dev/kinesis-kpl-consumer-deaggregation.html Kinesis provides an `AggregatorUtil` (https://github.com/awslabs/amazon-kinesis-client/blob/master/amazon-kinesis-client/src/main/java/software/amazon/kinesis/retrieval/AggregatorUtil.java) that can be used in the `KinesisConsumer` implementation. An example usage of this util can be found in the beam repo (https://github.com/apache/beam/blob/master/sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/kinesis/SimplifiedKinesisClient.java#L269) Even though the records in the batch have the same sequence number, we can append the sub-sequence number to Pinot kinesis' `StreamMessageOffset`. Changes should be fairly trivial to do this. Labels: `enhancement` , `kinesis`, `help-wanted` -- 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.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
[GitHub] [pinot] richardstartin commented on pull request #10151: Simplify UTF-8 comparison on null character
richardstartin commented on PR #10151: URL: https://github.com/apache/pinot/pull/10151#issuecomment-1397748773 Does `LoaderTest.testPadding` pass with this change? I'm all for simplification (assuming the whatever the motivation for adding `LoaderTest.testPadding` was no longer matters) but this isn't going to move the needle much performance wise. -- 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
[GitHub] [pinot] walterddr commented on a diff in pull request #10151: Simplify UTF-8 comparison on null character
walterddr commented on code in PR #10151:
URL: https://github.com/apache/pinot/pull/10151#discussion_r1081975613
##
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/readerwriter/ValueReaderComparisonTest.java:
##
@@ -156,22 +156,27 @@ void testSuperiorPrefixes(ValueReader reader, int
numBytesPerValue, String... st
void testConsistent(ValueReader reader, int numBytesPerValue, int
numValuesToCompare) {
for (int i = 0; i < numValuesToCompare; i++) {
byte[] bytes = new
byte[ThreadLocalRandom.current().nextInt(numBytesPerValue * 2)];
-assertConsistentUtf8Comparison(reader, i, numBytesPerValue, new
String(bytes, StandardCharsets.UTF_8));
-for (int j = 0; j < 128; j++) {
+
+for (int j = 1; j < 128; j++) {
Review Comment:
e.g. all zero fill data are not considered to be valid?
--
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
[GitHub] [pinot] navina commented on a diff in pull request #10136: Decouple dependency on using peer segment download scheme for doing metadata pushes to controller
navina commented on code in PR #10136:
URL: https://github.com/apache/pinot/pull/10136#discussion_r1081975469
##
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##
@@ -49,9 +49,13 @@ public class SegmentsValidationAndRetentionConfig extends
BaseJsonConfig {
// For more usage of this field, please refer to this design doc:
https://tinyurl.com/f63ru4sb
private String _peerSegmentDownloadScheme;
+ // Indicates if the segment should be uploaded to the deep store's file
system or to the controller during the
+ // segment commit protocol. By default, segment is uploaded to the
controller during commit.
+ // If this flag is set to true, the segment is uploaded to deep store.
+ private boolean _uploadToFileSystem = false;
Review Comment:
> For clarity, also suggest renaming to _serverUploadToDeepStore because we
want to emphasize that we want the server to do the upload
This makes sense.
> this flag belongs to the stream ingestion config, suggest moving it to
StreamIngestionConfig
yeah. I also agree it fits better in stream ingestion config. Should peer
segment download scheme remain in `SegmentsValidationAndRetentionConfig` ?
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10151: Simplify UTF-8 comparison on null character
codecov-commenter commented on PR #10151: URL: https://github.com/apache/pinot/pull/10151#issuecomment-1397743731 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10151](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (a2f44ed) into [master](https://codecov.io/gh/apache/pinot/commit/d4356b47ede99351625426a08117e18c00facadb?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (d4356b4) will **decrease** coverage by `54.97%`. > The diff coverage is `0.00%`. ```diff @@ Coverage Diff @@ ## master #10151 +/- ## = - Coverage 68.60% 13.64% -54.97% + Complexity 5770 176 -5594 = Files 2006 1951 -55 Lines108689 106199 -2490 Branches 1649316196 -297 = - Hits 7456314487-60076 - Misses2883890570+61732 + Partials 5288 1142 -4146 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `?` | | | unittests1 | `?` | | | unittests2 | `13.64% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [.../segment/local/io/util/ValueReaderComparisons.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pby91dGlsL1ZhbHVlUmVhZGVyQ29tcGFyaXNvbnMuamF2YQ==) | `0.00% <0.00%> (-97.02%)` | :arrow_down: | | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/spi/utils/LoopUtils.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTG9vcFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/common/CustomObject.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vQ3VzdG9tT2JqZWN0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/10151?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | |
[GitHub] [pinot] Jackie-Jiang merged pull request #9957: [multistage] [feature] Add a query option to pass some v1 limit
Jackie-Jiang merged PR #9957: URL: https://github.com/apache/pinot/pull/9957 -- 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
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #10151: Simplify UTF-8 comparison on null character
Jackie-Jiang opened a new pull request, #10151: URL: https://github.com/apache/pinot/pull/10151 Null character (byte zero) is not allowed in string values ingested into Pinot (all the string values ingested will be sanitized and all the characters after the first null character will be truncated), so we don't really need to guarantee the ordering when the input string value from the query has null character in the end because the original value is anyway modified. This can simplify the comparison logic and reduce overhead when string dictionary has paddings. E.g. if the value in the dictionary is "abc\0\0\0" after padding, the original input value could be "abc", "abc\0", "abc\0\0", "abc\0\0\0" or even "abc\0\0\0\0", "abc\0abc". After the change, the comparison result is: - = "abc" - = "abc\0" - = "abc\0\0" - = "abc\0\0\0" - < "abc\0\0\0\0" - < "abc\0a" -- 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
[GitHub] [pinot] agavra commented on a diff in pull request #10120: [multistage] support aggregations that require intermediate representations
agavra commented on code in PR #10120:
URL: https://github.com/apache/pinot/pull/10120#discussion_r1081932230
##
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotReduceAggregateFunctionsRule.java:
##
@@ -0,0 +1,201 @@
+/**
+ * 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.calcite.rel.rules;
+
+import com.google.common.collect.ImmutableSet;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.core.Aggregate;
+import org.apache.calcite.rel.core.AggregateCall;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.fun.PinotFourthMomentAggregateFunction;
+import org.apache.calcite.sql.fun.PinotKurtosisAggregateFunction;
+import org.apache.calcite.sql.fun.PinotOperatorTable;
+import org.apache.calcite.sql.fun.PinotSkewnessAggregateFunction;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.tools.RelBuilderFactory;
+import org.apache.calcite.util.CompositeList;
+
+
+/**
+ * This rule rewrites aggregate functions when necessary for Pinot's
+ * multistage engine. For example, SKEWNESS must be rewritten into two
+ * parts: a multi-stage FOURTH_MOMENT calculation and then a scalar function
+ * that reduces the moment into the skewness at the end. This is to ensure
+ * that the aggregation computation can merge partial results from different
+ * intermediate nodes before reducing it into the final result.
+ *
+ * This implementation follows closely with Calcite's
+ * {@link AggregateReduceFunctionsRule}.
+ */
+public class PinotReduceAggregateFunctionsRule extends RelOptRule {
Review Comment:
I spent some time trying to write a unit test for this, but unfortunately
Calcite things are impossibly difficult to test/mock. It ended up being tens of
lines of mocking and still I wasn't able to get it to work...
I think the easiest way to test this would be to create the EXPLAIN PLAN
test framework mentioned above.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10136: Decouple dependency on using peer segment download scheme for doing metadata pushes to controller
Jackie-Jiang commented on code in PR #10136:
URL: https://github.com/apache/pinot/pull/10136#discussion_r1081822321
##
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##
@@ -49,9 +49,13 @@ public class SegmentsValidationAndRetentionConfig extends
BaseJsonConfig {
// For more usage of this field, please refer to this design doc:
https://tinyurl.com/f63ru4sb
private String _peerSegmentDownloadScheme;
+ // Indicates if the segment should be uploaded to the deep store's file
system or to the controller during the
+ // segment commit protocol. By default, segment is uploaded to the
controller during commit.
+ // If this flag is set to true, the segment is uploaded to deep store.
+ private boolean _uploadToFileSystem = false;
Review Comment:
I think this flag belongs to the stream ingestion config, suggest moving it
to `StreamIngestionConfig`.
For clarity, also suggest renaming to `_serverUploadToDeepStore` because we
want to emphasize that we want the server to do the upload
--
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
[GitHub] [pinot] agavra commented on a diff in pull request #10120: [multistage] support aggregations that require intermediate representations
agavra commented on code in PR #10120:
URL: https://github.com/apache/pinot/pull/10120#discussion_r1081825016
##
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##
@@ -65,6 +65,7 @@ public enum AggregationFunctionType {
STDDEVSAMP("stdDevSamp"),
SKEWNESS("skewness"),
KURTOSIS("kurtosis"),
+ FOURTHMOMENT("fourthmoment"),
Review Comment:
yes, that's correct
--
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
[GitHub] [pinot] navina commented on pull request #10144: Adding duration unit in trivy timeout
navina commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1397475658 > I understand that the initial idea was to increase the timeout but we are using this PR to change the way trivy is triggered. Therefore I would suggest to change the title of the PR Yes. I will update the title, if needed before merging. -- 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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10089: Reload conditional skip
saurabhd336 commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1081698660
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##
@@ -403,6 +417,16 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
}
+ public boolean canReuseExistingDirectoryForReload(SegmentZKMetadata
segmentZKMetadata,
+ String currentSegmentTier, SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
+ Schema schema)
+ throws Exception {
+SegmentDirectoryLoader segmentDirectoryLoader =
+
SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader(indexLoadingConfig.getSegmentDirectoryLoader());
+return
!segmentDirectoryLoader.needsTierMigration(segmentZKMetadata.getTier(),
currentSegmentTier)
+&& !ImmutableSegmentLoader.needPreprocess(segmentDirectory,
indexLoadingConfig, schema);
Review Comment:
Isn't whether or not tier migration will actually happen a function of both
the zk tier being different from local segment metadata tier AND whether we're
using a locader that actually performs a tier migration?
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10089: Reload conditional skip
saurabhd336 commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1081688733
##
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##
@@ -191,7 +191,6 @@ public void testReloadSegmentUseLocalCopy()
fail();
} catch (Exception e) {
// As expected, segment reloading fails due to missing the local segment
dir.
- assertTrue(e.getMessage().contains("does not exist or is not a
directory"));
Review Comment:
Still fails with an exception (If it didn't, test would fail (see
https://github.com/apache/pinot/pull/10089/files#diff-2ca49ed375e7b21a34397df398e1586e8ee64283461abe4a9a2cae17097fbab9R191))
Just not the one with the message `does not exist or is not a directory`...
##
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##
@@ -191,7 +191,6 @@ public void testReloadSegmentUseLocalCopy()
fail();
} catch (Exception e) {
// As expected, segment reloading fails due to missing the local segment
dir.
- assertTrue(e.getMessage().contains("does not exist or is not a
directory"));
Review Comment:
Same for next one
--
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
[GitHub] [pinot] walterddr commented on issue #10129: [multistage] Projection Not Pushed Down for Agg Queries with No Grouping Sets
walterddr commented on issue #10129: URL: https://github.com/apache/pinot/issues/10129#issuecomment-1397438501 did a bit of research on this topic: the problem only occurs on `COUNT(*)` or `COUNT(1)`, `COUNT(col)` this is b/c calcite will optimize out the argument since all columns in pinot are marked non-nullable. 1. several attempts have been tried on this one but the most promising so far is to replace `COUNT` with `SUM(1)` thus bypass calcite's optimization. with this project pushdown are being executed properly. 2. @ankitsultana also reported that the pushdown doesn't apply to semi JOINs with NOT-IN clause properly ``` SELECT SUM(1) FROM baseballStats_OFFLINE WHERE hits > 10 AND playerID NOT IN (SELECT playerID FROM baseballStats_OFFLINE WHERE hits < 5) ``` ^ this seems to be related to the fact that NOT IN produces a in-equality JOIN condition that cannot be optimized into a proper JOIN and thus cannot apply proper pushdown rules. which we will address separately. -- 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
[GitHub] [pinot] 61yao commented on a diff in pull request #9957: [multistage] [feature] Add a query option to pass some v1 limit
61yao commented on code in PR #9957:
URL: https://github.com/apache/pinot/pull/9957#discussion_r1081642038
##
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##
@@ -99,6 +99,14 @@ public void testGeneratedQueries()
super.testGeneratedQueries(false, true);
}
+ @Test
+ public void testQueryOptions()
+ throws Exception {
+String pinotQuery = "SET multistageLeafLimit = 1; SELECT * FROM mytable;";
+String h2Query = "SELECT * FROM mytable";
Review Comment:
Added a test to compare # of rows. PTAL.
--
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
[GitHub] [pinot] snleee merged pull request #10106: clean up the logic of reading guage metrics
snleee merged PR #10106: URL: https://github.com/apache/pinot/pull/10106 -- 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
[GitHub] [pinot] npawar commented on pull request #10144: Adding duration unit in trivy timeout
npawar commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1397373177 Not sure if periodic task is the best way to go forward here. I'm okay if we merge the flagged PR, and then separately fix the vulnerability detected (either the same PR author does it, or reports to pinot-contributers channel based on what the dependency is that caused the failure). But at least that way, we'll have someone in the loop about it. -- 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
[GitHub] [pinot] zhtaoxiang commented on a diff in pull request #10106: clean up the logic of reading guage metrics
zhtaoxiang commented on code in PR #10106:
URL: https://github.com/apache/pinot/pull/10106#discussion_r1081609829
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##
@@ -481,20 +461,6 @@ public long getValueOfTableGauge(final String tableName,
final G gauge) {
return gaugeValue == null ? 0 : gaugeValue.get();
}
- /**
- * Gets the value of a table partition gauge.
- *
- * @param tableName The table name
- * @param partitionId The partition name
- * @param gauge The gauge to use
- */
- public long getValueOfPartitionGauge(final String tableName, final int
partitionId, final G gauge) {
-final String fullGaugeName = composeTableGaugeName(tableName,
String.valueOf(partitionId), gauge);
-
-AtomicLong gaugeValue = _gaugeValues.get(fullGaugeName);
-return gaugeValue == null ? -1 : gaugeValue.get();
Review Comment:
This code is removed. So no way to add `@VisibleForTesting`
--
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
[GitHub] [pinot] zhtaoxiang commented on a diff in pull request #10106: clean up the logic of reading guage metrics
zhtaoxiang commented on code in PR #10106:
URL: https://github.com/apache/pinot/pull/10106#discussion_r1081607958
##
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java:
##
@@ -0,0 +1,128 @@
+/**
+ * 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.metrics;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerMetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerSettableGauge;
+import org.apache.pinot.spi.metrics.PinotMetric;
+
+
+public class MetricValueUtils {
+ private MetricValueUtils() {
+ }
+
+ public static boolean gaugeExists(AbstractMetrics metrics, String
metricName) {
+return extractMetric(metrics, metricName) != null;
+ }
+
+ public static long getGaugeValue(AbstractMetrics metrics, String metricName)
{
+PinotMetric pinotMetric = extractMetric(metrics, metricName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName()) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName());
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + key) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName) !=
null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ key) != null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean partitionGaugeExists(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ partitionId) != null;
+ }
+
+ public static long getPartitionGaugeValue(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + partitionId);
+if (pinotMetric == null) {
Review Comment:
The code is defined under `src/test` path, so it's by default visible to
tests only
--
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
[GitHub] [pinot] klsince commented on a diff in pull request #10089: Reload conditional skip
klsince commented on code in PR #10089:
URL: https://github.com/apache/pinot/pull/10089#discussion_r1081607056
##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##
@@ -207,11 +207,7 @@ private boolean needProcessStarTrees() {
List starTreeMetadataList =
_segmentMetadata.getStarTreeV2MetadataList();
// There are existing star-trees, but if they match the builder configs
exactly,
// then there is no need to generate the star-trees
-if (starTreeMetadataList != null && !StarTreeBuilderUtils
-.shouldRemoveExistingStarTrees(starTreeBuilderConfigs,
starTreeMetadataList)) {
- return false;
-}
-return !starTreeBuilderConfigs.isEmpty();
+return
StarTreeBuilderUtils.existingStarTreesNeedChange(starTreeBuilderConfigs,
starTreeMetadataList);
Review Comment:
bump ^
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##
@@ -403,6 +417,16 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
}
}
+ public boolean canReuseExistingDirectoryForReload(SegmentZKMetadata
segmentZKMetadata,
+ String currentSegmentTier, SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
+ Schema schema)
+ throws Exception {
+SegmentDirectoryLoader segmentDirectoryLoader =
+
SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader(indexLoadingConfig.getSegmentDirectoryLoader());
+return
!segmentDirectoryLoader.needsTierMigration(segmentZKMetadata.getTier(),
currentSegmentTier)
+&& !ImmutableSegmentLoader.needPreprocess(segmentDirectory,
indexLoadingConfig, schema);
Review Comment:
How about put the method on SegmentDirectory iface? as segmentDirectory
tracks its current tier, so can compare with the new tier. Custom
segmentDirectory impls can decide whether to migrate tier per need.
```
return segmentDirectory.needTierMigration(segmentZKMetadata.getTier()) && ...
```
##
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##
@@ -191,7 +191,6 @@ public void testReloadSegmentUseLocalCopy()
fail();
} catch (Exception e) {
// As expected, segment reloading fails due to missing the local segment
dir.
- assertTrue(e.getMessage().contains("does not exist or is not a
directory"));
Review Comment:
did reloadSegment() complete w/o any errors? that's a bit unexpected as L188
deleted the local seg dir. same question for next test case.
--
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
[GitHub] [pinot] walterddr commented on pull request #10148: [multistage] flag all columns as nullable
walterddr commented on PR #10148: URL: https://github.com/apache/pinot/pull/10148#issuecomment-1397353721 this have introduce new set of issues. so we really need a proper null detection on columns. will shelf this PR for 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
[GitHub] [pinot] jugomezv commented on a diff in pull request #10106: clean up the logic of reading guage metrics
jugomezv commented on code in PR #10106:
URL: https://github.com/apache/pinot/pull/10106#discussion_r1081597788
##
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java:
##
@@ -0,0 +1,128 @@
+/**
+ * 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.metrics;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerMetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerSettableGauge;
+import org.apache.pinot.spi.metrics.PinotMetric;
+
+
+public class MetricValueUtils {
+ private MetricValueUtils() {
+ }
+
+ public static boolean gaugeExists(AbstractMetrics metrics, String
metricName) {
+return extractMetric(metrics, metricName) != null;
+ }
+
+ public static long getGaugeValue(AbstractMetrics metrics, String metricName)
{
+PinotMetric pinotMetric = extractMetric(metrics, metricName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName()) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName());
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + key) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName) !=
null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ key) != null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean partitionGaugeExists(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ partitionId) != null;
+ }
+
+ public static long getPartitionGaugeValue(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + partitionId);
+if (pinotMetric == null) {
Review Comment:
This return -1 is strange but @zhtaoxiang is right on this being used only
on testing, just nit would be marked as visible for testing only as suggested
above
--
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
[GitHub] [pinot] jugomezv commented on a diff in pull request #10106: clean up the logic of reading guage metrics
jugomezv commented on code in PR #10106:
URL: https://github.com/apache/pinot/pull/10106#discussion_r1081595797
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##
@@ -481,20 +461,6 @@ public long getValueOfTableGauge(final String tableName,
final G gauge) {
return gaugeValue == null ? 0 : gaugeValue.get();
}
- /**
- * Gets the value of a table partition gauge.
- *
- * @param tableName The table name
- * @param partitionId The partition name
- * @param gauge The gauge to use
- */
- public long getValueOfPartitionGauge(final String tableName, final int
partitionId, final G gauge) {
-final String fullGaugeName = composeTableGaugeName(tableName,
String.valueOf(partitionId), gauge);
-
-AtomicLong gaugeValue = _gaugeValues.get(fullGaugeName);
-return gaugeValue == null ? -1 : gaugeValue.get();
Review Comment:
So should this be Visible for testing only?
--
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
[GitHub] [pinot] walterddr merged pull request #10094: [multistage] [debuggability] OpChain and operator stats
walterddr merged PR #10094: URL: https://github.com/apache/pinot/pull/10094 -- 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
[GitHub] [pinot] walterddr commented on a diff in pull request #10144: Adding duration unit in trivy timeout
walterddr commented on code in PR #10144: URL: https://github.com/apache/pinot/pull/10144#discussion_r1081538383 ## .github/workflows/pinot_vuln_check.yml: ## @@ -19,18 +19,8 @@ name: Pinot Dependencies on: - push: -branches: - - master - pull_request: -branches: - - master -paths: - - "**/pom.xml" - - "**/package.json" - - "**/package-lock.json" - - "docker/images/pinot/**" - - ".github/workflows/**" + schedule: Review Comment: i would say let's be more restrictive in the glob pattern when triggering this vuln check. for example package-lock.json, DOCKERFILE, pom.xml i think all the problem originated from the fact that all files under docker image folder was matched -- 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
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10148: [multistage] flag all columns as nullable
ankitsultana commented on code in PR #10148:
URL: https://github.com/apache/pinot/pull/10148#discussion_r1081312765
##
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java:
##
@@ -54,23 +54,23 @@ public RelDataType createRelDataTypeFromSchema(Schema
schema) {
private RelDataType toRelDataType(FieldSpec fieldSpec) {
switch (fieldSpec.getDataType()) {
case INT:
-return createSqlType(SqlTypeName.INTEGER);
+return createTypeWithNullability(createSqlType(SqlTypeName.INTEGER),
true);
Review Comment:
Can you check this patch with the COLOCATED_JOIN quickstart and this query?
I saw a broadcast distributed stage created for some reason. Also the
projection wasn't pushed down to the userAttributes side.
```
SELECT
COUNT(*)
FROM
userAttributes_OFFLINE
WHERE
userUUID NOT IN (
SELECT userUUID FROM userGroups_OFFLINE WHERE groupUUID = 'group-2'
)
```
--
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
[GitHub] [pinot] ankitsultana commented on issue #10140: [multistage] Expensive RoundRobinScheduler::hasNext Calls
ankitsultana commented on issue #10140: URL: https://github.com/apache/pinot/issues/10140#issuecomment-1396973491 Side-note: I think entries in seenMail may get leaked if `_releaseTs` is set to a finite value. The reason is that the OpChain may hang up and the sender may send the data after the corresponding OpChain has already finished as far as the scheduler is considered. It's only a small leak but a leak nevertheless. Broadly speaking I think we need to gracefully handle the cases where a sender is ready to send data to a stage but the receiver either hasn't been registered yet or has already hung up. > Separately, we should also see if we can bound the GRPC worker threads +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
[GitHub] [pinot] Ferix9288 commented on pull request #10139: JMX to Prom Exporter - More Inclusive Table Regex
Ferix9288 commented on PR #10139:
URL: https://github.com/apache/pinot/pull/10139#issuecomment-1396918251
Thanks @richardstartin for the input!
@Jackie-Jiang I added `\\S+` regex to all other instances in which there was
a `table` label; good catch on that. Also are we sure we should let whitespaces
for table names? Wouldn't that kill a lot of the controller APIs that requires
{{tableName}}? Or would encoding it as `%20` work? I am not sure if Pinot
Controller UI would handle all of that correctly.
Sidenote: I created my own pinot custom image that has my changes baked in.
Can confirm that this fixes our usecase and issue mentioned.
--
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
[GitHub] [pinot] xiangfu0 merged pull request #10149: Fixing the metadata tar gz file path
xiangfu0 merged PR #10149: URL: https://github.com/apache/pinot/pull/10149 -- 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
[GitHub] [pinot] richardstartin commented on pull request #10139: JMX to Prom Exporter - More Inclusive Table Regex
richardstartin commented on PR #10139: URL: https://github.com/apache/pinot/pull/10139#issuecomment-1396760434 I don't have much of an opinion on what these regular expressions should be, I just took what was there before (which was working for us) and split it into different files so we don't have server rules on the broker and so on. The changes here look good. -- 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
[GitHub] [pinot] Ferix9288 commented on pull request #10139: JMX to Prom Exporter - More Inclusive Table Regex
Ferix9288 commented on PR #10139: URL: https://github.com/apache/pinot/pull/10139#issuecomment-1396699401 Hey @Jackie-Jiang , much appreciated for the prompt review! Ah very nice. So possibly a non-greedy search like (.*?)_(\w+)? And np, I will fix the other regexes that also contain label `table` (but without the suffix) once we agree upon the new regex to match the table name more liberally. > When the table name contains _, can it correctly match the name? Ah that's an excellent point. Yes, if a table name was something like `fake_table`, then regex will match this and deem it valid. However, code-wise and from what I've witnessed, the proper suffix is always appended to it. We could make it: `(.*?)_(OFFLINE|REALTIME)` and be more explicit though. -- 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
[GitHub] [pinot] gortiz commented on pull request #10144: Adding duration unit in trivy timeout
gortiz commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1396567329 I understand that the initial idea was to increase the timeout but we are using this PR to change the way trivy is triggered. Therefore I would suggest to change the title of 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
[GitHub] [pinot] gortiz commented on a diff in pull request #10144: Adding duration unit in trivy timeout
gortiz commented on code in PR #10144: URL: https://github.com/apache/pinot/pull/10144#discussion_r1080906416 ## .github/workflows/pinot_vuln_check.yml: ## @@ -19,18 +19,8 @@ name: Pinot Dependencies on: - push: -branches: - - master - pull_request: -branches: - - master -paths: - - "**/pom.xml" - - "**/package.json" - - "**/package-lock.json" - - "docker/images/pinot/**" - - ".github/workflows/**" + schedule: Review Comment: We were talking about changing this check to do not run on each push but run periodically in order to do not mark as erroneous PRs that do not actually introduce the vulnerability. On the other hand, we would lose the ability to detect when a PR adds a known vulnerability. Instead we would need to be notified by this periodic cron. The big question here is how are we going to know when this cron fails. -- 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
[GitHub] [pinot] codecov-commenter commented on pull request #10150: Fix json-smart version
codecov-commenter commented on PR #10150: URL: https://github.com/apache/pinot/pull/10150#issuecomment-1396548970 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10150](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (fbe6dd1) into [master](https://codecov.io/gh/apache/pinot/commit/21c6532564cb0fea682cf362a4d4eec37ef831e4?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (21c6532) will **decrease** coverage by `7.64%`. > The diff coverage is `n/a`. ```diff @@ Coverage Diff @@ ## master #10150 +/- ## - Coverage 32.22% 24.59% -7.64% + Complexity 191 44 -147 Files 2005 1993 -12 Lines108548 108195 -353 Branches 1648816450 -38 - Hits 3498526608-8377 - Misses7045478809+8355 + Partials 3109 2778 -331 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `24.59% <ø> (-0.20%)` | :arrow_down: | | unittests2 | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../org/apache/pinot/client/AggregationResultSet.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0FnZ3JlZ2F0aW9uUmVzdWx0U2V0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ntroller/recommender/rules/impl/JsonIndexRule.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0pzb25JbmRleFJ1bGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...troller/recommender/io/metadata/FieldMetadata.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9tZXRhZGF0YS9GaWVsZE1ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...roller/recommender/rules/impl/BloomFilterRule.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0Jsb29tRmlsdGVyUnVsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...oller/api/resources/PinotControllerAppConfigs.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90Q29udHJvbGxlckFwcENvbmZpZ3MuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ler/recommender/data/generator/BytesGenerator.java](https://codecov.io/gh/apache/pinot/pull/10150?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9kYXRhL2dlbmVyYXRvci9CeXRlc0dlbmVyYXRvci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | |
[GitHub] [pinot] codecov-commenter commented on pull request #10149: Fixing the metadata tar gz file path
codecov-commenter commented on PR #10149: URL: https://github.com/apache/pinot/pull/10149#issuecomment-1396526998 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10149](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (61bc28a) into [master](https://codecov.io/gh/apache/pinot/commit/21c6532564cb0fea682cf362a4d4eec37ef831e4?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (21c6532) will **increase** coverage by `28.97%`. > The diff coverage is `83.33%`. ```diff @@ Coverage Diff @@ ## master #10149 +/- ## = + Coverage 32.22% 61.20% +28.97% - Complexity 191 4977 +4786 = Files 2005 1993 -12 Lines108548 108205 -343 Branches 1648816450 -38 = + Hits 3498566222+31237 + Misses7045437022-33432 - Partials 3109 4961 +1852 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `24.64% <0.00%> (-0.15%)` | :arrow_down: | | unittests1 | `67.81% <83.33%> (?)` | | | unittests2 | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...he/pinot/segment/local/utils/SegmentPushUtils.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9TZWdtZW50UHVzaFV0aWxzLmphdmE=) | `15.27% <83.33%> (+15.27%)` | :arrow_up: | | [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../org/apache/pinot/client/AggregationResultSet.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0FnZ3JlZ2F0aW9uUmVzdWx0U2V0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ntroller/recommender/rules/impl/JsonIndexRule.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0pzb25JbmRleFJ1bGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...troller/recommender/io/metadata/FieldMetadata.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9tZXRhZGF0YS9GaWVsZE1ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...roller/recommender/rules/impl/BloomFilterRule.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0Jsb29tRmlsdGVyUnVsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...oller/api/resources/PinotControllerAppConfigs.java](https://codecov.io/gh/apache/pinot/pull/10149?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90Q29udHJvbGxlckFwcENvbmZpZ3MuamF2YQ==) | `0.00% <0.00%> (-100.00%)`
[GitHub] [pinot] zhtaoxiang commented on a diff in pull request #10106: clean up the logic of reading guage metrics
zhtaoxiang commented on code in PR #10106:
URL: https://github.com/apache/pinot/pull/10106#discussion_r1080869461
##
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java:
##
@@ -0,0 +1,128 @@
+/**
+ * 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.metrics;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerMetricName;
+import org.apache.pinot.plugin.metrics.yammer.YammerSettableGauge;
+import org.apache.pinot.spi.metrics.PinotMetric;
+
+
+public class MetricValueUtils {
+ private MetricValueUtils() {
+ }
+
+ public static boolean gaugeExists(AbstractMetrics metrics, String
metricName) {
+return extractMetric(metrics, metricName) != null;
+ }
+
+ public static long getGaugeValue(AbstractMetrics metrics, String metricName)
{
+PinotMetric pinotMetric = extractMetric(metrics, metricName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName()) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName());
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean globalGaugeExists(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + key) != null;
+ }
+
+ public static long getGlobalGaugeValue(AbstractMetrics metrics, String key,
AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName) !=
null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean tableGaugeExists(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ key) != null;
+ }
+
+ public static long getTableGaugeValue(AbstractMetrics metrics, String
tableName, String key,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + key);
+if (pinotMetric == null) {
+ return 0;
+}
+return ((YammerSettableGauge) pinotMetric.getMetric()).value();
+ }
+
+ public static boolean partitionGaugeExists(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+return extractMetric(metrics, gauge.getGaugeName() + "." + tableName + "."
+ partitionId) != null;
+ }
+
+ public static long getPartitionGaugeValue(AbstractMetrics metrics, String
tableName, int partitionId,
+ AbstractMetrics.Gauge gauge) {
+PinotMetric pinotMetric = extractMetric(metrics, gauge.getGaugeName() +
"." + tableName + "." + partitionId);
+if (pinotMetric == null) {
Review Comment:
This `getValueOfPartitionGauge` is only used in unit tests and integration
tests, as long as we don't break those tests, it should be fine even if we
don't return `-1` in the new method
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##
@@ -481,20 +461,6 @@ public long getValueOfTableGauge(final String tableName,
final G gauge) {
return
[GitHub] [pinot] xiangfu0 opened a new pull request, #10150: Fix json-smart version
xiangfu0 opened a new pull request, #10150: URL: https://github.com/apache/pinot/pull/10150 Instructions: 1. The PR has to be tagged with at least one of the following labels (*): 1. `feature` 2. `bugfix` 3. `performance` 4. `ui` 5. `backward-incompat` 6. `release-notes` (**) 2. Remove these instructions before publishing the PR. (*) Other labels to consider: - `testing` - `dependencies` - `docker` - `kubernetes` - `observability` - `security` - `code-style` - `extension-point` - `refactor` - `cleanup` (**) Use `release-notes` label for scenarios like: - New configuration options - Deprecation of configurations - Signature changes to public methods/interfaces - New plugins added or old plugins removed -- 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
[GitHub] [pinot] xiangfu0 commented on pull request #10144: Adding duration unit in trivy timeout
xiangfu0 commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1396496351 My only concern is that who will be responsible for the trivy check results and version bump. -- 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
[GitHub] [pinot] codecov-commenter commented on pull request #10148: [multistage] flag all columns as nullable
codecov-commenter commented on PR #10148: URL: https://github.com/apache/pinot/pull/10148#issuecomment-1396491709 # [Codecov](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#10148](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (fd61cb7) into [master](https://codecov.io/gh/apache/pinot/commit/eaa60d93fb032114db618d2f0ef329e6f8fb7cf2?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (eaa60d9) will **decrease** coverage by `11.09%`. > The diff coverage is `0.00%`. ```diff @@ Coverage Diff @@ ## master #10148 +/- ## = - Coverage 24.75% 13.66% -11.09% - Complexity 44 176 +132 = Files 1988 1950 -38 Lines108125 106062 -2063 Branches 1644616194 -252 = - Hits 2676314491-12272 - Misses7856090428+11868 + Partials 2802 1143 -1659 ``` | Flag | Coverage Δ | | |---|---|---| | integration1 | `?` | | | unittests2 | `13.66% <0.00%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [.../java/org/apache/pinot/query/type/TypeFactory.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdHlwZS9UeXBlRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (ø)` | | | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [.../java/org/apache/pinot/common/auth/AuthConfig.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXV0aC9BdXRoQ29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/10148?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00%
[GitHub] [pinot] navina commented on a diff in pull request #10144: Adding duration unit in trivy timeout
navina commented on code in PR #10144: URL: https://github.com/apache/pinot/pull/10144#discussion_r1080845287 ## .github/workflows/pinot_vuln_check.yml: ## @@ -19,18 +19,8 @@ name: Pinot Dependencies on: - push: -branches: - - master - pull_request: -branches: - - master -paths: - - "**/pom.xml" - - "**/package.json" - - "**/package-lock.json" - - "docker/images/pinot/**" - - ".github/workflows/**" + schedule: Review Comment: @walterddr trigger based on what ? can you tell me how? -- 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
[GitHub] [pinot] navina commented on pull request #10136: Decouple dependency on using peer segment download scheme for doing metadata pushes to controller
navina commented on PR #10136: URL: https://github.com/apache/pinot/pull/10136#issuecomment-1396486769 @Jackie-Jiang I don't think we need 2 new configs, as we discussed. We can make do with 1 new config. Please review! -- 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
[GitHub] [pinot] xiangfu0 opened a new pull request, #10149: Fixing the metadata tar gz file path
xiangfu0 opened a new pull request, #10149: URL: https://github.com/apache/pinot/pull/10149 Fixing https://apache-pinot.slack.com/archives/C011C9JHN7R/p1674101384733239 -- 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
[GitHub] [pinot] walterddr opened a new pull request, #10148: [multistage] flag all columns as nullable
walterddr opened a new pull request, #10148: URL: https://github.com/apache/pinot/pull/10148 currently, there's no way to tell whether a table column is nullable setting the nullability for all columns on v2 engine as default true (previously default false), b/c - having nullability default to false causes some incorrect optimizer rules to apply. - this also help mitigating the issue with #10129 -- 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
[GitHub] [pinot] shwin commented on a diff in pull request #10034: Create metadata only tarball for metadata push job
shwin commented on code in PR #10034:
URL: https://github.com/apache/pinot/pull/10034#discussion_r1080812390
##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##
@@ -284,7 +284,22 @@ public static void
sendSegmentUriAndMetadata(SegmentGenerationJobSpec spec, Pino
String segmentName = fileName.endsWith(Constants.TAR_GZ_FILE_EXT)
? fileName.substring(0, fileName.length() -
Constants.TAR_GZ_FILE_EXT.length()) : fileName;
SegmentNameUtils.validatePartialOrFullSegmentName(segmentName);
- File segmentMetadataFile = generateSegmentMetadataFile(fileSystem,
URI.create(tarFilePath));
+ File segmentMetadataFile;
+ // Check if there is a segment metadata tar gz file named
`segmentName.metadata.tar.gz`, already in the remote
+ // directory. This is to avoid generating a new segment metadata tar gz
file every time we push a segment,
+ // which requires downloading the entire segment tar gz file.
+ URI metadataTarGzFilePath = URI.create(
+ new File(tarFilePath).getParentFile() + File.separator + segmentName
+ Constants.METADATA_TAR_GZ_FILE_EXT);
+ if (spec.getPushJobSpec().isPreferMetadataTarGz() &&
fileSystem.exists(metadataTarGzFilePath)) {
Review Comment:
I think https://replit.com/join/kndnjhlrzw-ashwinraja2 shows what's going on
--
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
[GitHub] [pinot] shwin commented on a diff in pull request #10034: Create metadata only tarball for metadata push job
shwin commented on code in PR #10034:
URL: https://github.com/apache/pinot/pull/10034#discussion_r1080811688
##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##
@@ -284,7 +284,22 @@ public static void
sendSegmentUriAndMetadata(SegmentGenerationJobSpec spec, Pino
String segmentName = fileName.endsWith(Constants.TAR_GZ_FILE_EXT)
? fileName.substring(0, fileName.length() -
Constants.TAR_GZ_FILE_EXT.length()) : fileName;
SegmentNameUtils.validatePartialOrFullSegmentName(segmentName);
- File segmentMetadataFile = generateSegmentMetadataFile(fileSystem,
URI.create(tarFilePath));
+ File segmentMetadataFile;
+ // Check if there is a segment metadata tar gz file named
`segmentName.metadata.tar.gz`, already in the remote
+ // directory. This is to avoid generating a new segment metadata tar gz
file every time we push a segment,
+ // which requires downloading the entire segment tar gz file.
+ URI metadataTarGzFilePath = URI.create(
+ new File(tarFilePath).getParentFile() + File.separator + segmentName
+ Constants.METADATA_TAR_GZ_FILE_EXT);
+ if (spec.getPushJobSpec().isPreferMetadataTarGz() &&
fileSystem.exists(metadataTarGzFilePath)) {
Review Comment:
Mentioned this in slack
(https://apache-pinot.slack.com/archives/C011C9JHN7R/p1674101587879929?thread_ts=1674101384.733239=C011C9JHN7R)
but I think this line doesn't work with S3 paths:
`File(tarFilePath).getParentFile()` will end up returning `s3:/bucket/path`
NOT `s3://bucket/path` which ends up messing the subsequent existence code.
--
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
[GitHub] [pinot] jasperjiaguo commented on issue #9277: [Feature] Support for Correlation Function
jasperjiaguo commented on issue #9277: URL: https://github.com/apache/pinot/issues/9277#issuecomment-1396440724 NaN could be due to Math.sqrt(negative_number) or 0.0/0.0 We have recently discovered this impl of covariance/correlation has numerical stability issue when E[x^2] ~ E[x]^2 >> 0 (see [1](https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Covariance) [2](https://github.com/trinodb/trino/blob/1866a23e3b0377144c1820de892c0de2762351a8/core/trino-main/src/main/java/io/trino/operator/aggregation/state/CorrelationState.java) [3](https://github.com/trinodb/trino/blob/1866a23e3b0377144c1820de892c0de2762351a8/core/trino-main/src/main/java/io/trino/operator/aggregation/state/CovarianceState.java)). Could we also use similar implementations? I'm not sure if the online algorithm is already available as a library, but feel free to use if apache common has it. -- 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
[GitHub] [pinot] sajjad-moradi commented on issue #10147: Pauseless Consumption
sajjad-moradi commented on issue #10147: URL: https://github.com/apache/pinot/issues/10147#issuecomment-1396417093 CC: @mcvsubbu @npawar @Jackie-Jiang -- 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
[GitHub] [pinot] sajjad-moradi opened a new issue, #10147: Pauseless Consumption
sajjad-moradi opened a new issue, #10147: URL: https://github.com/apache/pinot/issues/10147 # Background Currently when commit protocol starts for a partition, the realtime server stops consumption and till the new consuming->online helix message for the next consuming segment arrives, messages are not read from the stream. Depending on the segment size, the period in which there's no consumption can take something from few seconds to a several minutes (and in some cases tens of minutes because of hitting concurrent segment build threshold on the servers). This leads to data freshness issues specially for high ingesting use cases. # Proposal A simple solution to prevent these consumption pauses is to spin off a temporary consuming segment as soon as the segment commit starts. This temporary segment is a supplement to the main segment that is committing. There won't be any changes in Ideal State and External View. ## Query execution To make the ingested data in the temporary segment available for query execution, Server Query Executor sends the incoming query to the main segment as well as the temporary segment if there is one. Both segments execute the query and return the results. ## Next Offline to Consuming Transition After the main segment is committed and the next consuming segment is created during handling of offline->consuming helix transition message, the data in the temporary segment can be used to bootstrap the new consuming segment. Once the mutable segment is created for the next consuming segment, and before the consume loop starts, the data in the temporary segment can be read and inserted into the new mutable segment. After the data is copied over, the temporary segment can be deleted, and the consume loop in the new consuming segment starts from the offset of the last copied message. ## Configurations - We can add a new boolean flag to the Stream Config properties to enable/disable pause-less consumption. We can use something like `pauseless.consumption.enabled`. - For the size of the temporary segment, we can go with a default value of 20% of the main segment size. This ratio can be configured as a stream config property, something like `pauseless.consumption.temporary.segment.size.ratio`. - We also need to set a timeout for temporary consumption in case something is wrong and the helix transition message for the next consuming segment doesn't arrive to the server. We can have a default 10min value and also a new property in stream config to override that, something like `pauseless.consumption.timeout`. -- 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.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
[GitHub] [pinot] snleee opened a new issue, #10146: Add the support for `STDDEV_POP/SAMP, VAR_POP/DEV` for star tree
snleee opened a new issue, #10146: URL: https://github.com/apache/pinot/issues/10146 We recently added the `STDDEV_POP/SAMP, VAR_POP/DEV` aggregation function. It would be great if we can support this in the star tree index. We need to implement `ValueAggregator` interface for stddev and var. -- 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.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
[GitHub] [pinot] mcvsubbu commented on issue #10137: Determine Number of Rows for a Realtime Segment Based on a Config
mcvsubbu commented on issue #10137: URL: https://github.com/apache/pinot/issues/10137#issuecomment-1396336852 We should really think about configs that add no value. IMO this config adds no value. I think the non-expert users will come back asking us to tune this number because they rebalanced, or the number of partitions changed etc. It is one less config to document. This is my vote -- but sure, majority wins :) -- 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
[GitHub] [pinot] siddharthteotia commented on issue #9277: [Feature] Support for Correlation Function
siddharthteotia commented on issue #9277: URL: https://github.com/apache/pinot/issues/9277#issuecomment-1396328556 May be you are running into divide by 0 problem ? Did you try step into the code to understand where it is turning into NaN ? cc @jasperjiaguo / @SabrinaZhaozyf -- 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
[GitHub] [pinot] xiangfu0 merged pull request #10145: Fix docker build script tagging
xiangfu0 merged PR #10145: URL: https://github.com/apache/pinot/pull/10145 -- 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
[GitHub] [pinot] Jackie-Jiang commented on issue #10137: Determine Number of Rows for a Realtime Segment Based on a Config
Jackie-Jiang commented on issue #10137: URL: https://github.com/apache/pinot/issues/10137#issuecomment-1396323369 Most users are not using the recommendation engine though. What I'm trying to convey is that currently there is not a straight forward way to configure the rows per segment. Fixing rows per segment is the easiest way to guarantee good performance, and we should provide an easy way to configure it without knowing how things are managed internally. I agree using the more complex mechanism can give better stability, but we also want a simple configuration for non-expert users. -- 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
[GitHub] [pinot] xiangfu0 opened a new pull request, #10145: Fix docker build script tagging
xiangfu0 opened a new pull request, #10145: URL: https://github.com/apache/pinot/pull/10145 Fix the tagging variables generation from the build code. -- 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
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10120: [multistage] support aggregations that require intermediate representations
siddharthteotia commented on code in PR #10120:
URL: https://github.com/apache/pinot/pull/10120#discussion_r1080718243
##
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##
@@ -65,6 +65,7 @@ public enum AggregationFunctionType {
STDDEVSAMP("stdDevSamp"),
SKEWNESS("skewness"),
KURTOSIS("kurtosis"),
+ FOURTHMOMENT("fourthmoment"),
Review Comment:
cc @agavra
--
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
[GitHub] [pinot] siddharthteotia merged pull request #10120: [multistage] support aggregations that require intermediate representations
siddharthteotia merged PR #10120: URL: https://github.com/apache/pinot/pull/10120 -- 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
[GitHub] [pinot] siddharthteotia closed issue #10109: Support DistinctSum and DistinctAvg aggregation functions for MV columns
siddharthteotia closed issue #10109: Support DistinctSum and DistinctAvg aggregation functions for MV columns URL: https://github.com/apache/pinot/issues/10109 -- 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
[GitHub] [pinot] siddharthteotia merged pull request #10128: Support for DistinctSumMV and DistinctAvgMV aggregation functions
siddharthteotia merged PR #10128: URL: https://github.com/apache/pinot/pull/10128 -- 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
[GitHub] [pinot] siddharthteotia commented on pull request #10128: Support for DistinctSumMV and DistinctAvgMV aggregation functions
siddharthteotia commented on PR #10128: URL: https://github.com/apache/pinot/pull/10128#issuecomment-1396312237 only text / javadoc changed in the latest commit. -- 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
[GitHub] [pinot] mcvsubbu commented on issue #10137: Determine Number of Rows for a Realtime Segment Based on a Config
mcvsubbu commented on issue #10137: URL: https://github.com/apache/pinot/issues/10137#issuecomment-1396307559 > @mcvsubbu The proposed way is very useful when we want to limit the CPU usage for all the consumers on a given server, but for certain use cases, CPU might not be the bottleneck (e.g. ingestion rate is low, query throughput is low, no strict latency requirement), and we just want an easy way to configure the segment size so that segment is not too small or too big. It will be similar to the size (in bytes) based config, but we fix the number of rows per segment. It will be easier to config and get desired performance than the size based because for wide or narrow table, size based will give very different row count per segment. For columnar store, row count is the major factor of the query performance. Also, the size based config can only be properly set after the first several segments are consumed, which is not as straight-forward as the row based. I don't agree with that last part. The size is recommended by the recommendation engine, and can be configured in. -- 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
[GitHub] [pinot] 61yao commented on a diff in pull request #10094: [multistage] [debuggability] OpChain and operator stats
61yao commented on code in PR #10094:
URL: https://github.com/apache/pinot/pull/10094#discussion_r1080713806
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##
@@ -0,0 +1,77 @@
+/**
+ * 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.runtime.operator;
+
+import com.google.common.base.Stopwatch;
+import java.util.concurrent.TimeUnit;
+
+
+public class OperatorStats {
+ private final Stopwatch _executeStopwatch = Stopwatch.createUnstarted();
+
+ // TODO: add a operatorId for better tracking purpose.
+ private final int _stageId;
+ private final long _requestId;
+
+ private final String _operatorType;
+
+ private int _numInputBlock = 0;
+ private int _numInputRows = 0;
+
+ private int _numOutputBlock = 0;
+
+ private int _numOutputRows = 0;
+
+ public OperatorStats(long requestId, int stageId, String operatorType) {
+_stageId = stageId;
+_requestId = requestId;
+_operatorType = operatorType;
+ }
+
+ public void startTimer() {
+if(!_executeStopwatch.isRunning()){
+ _executeStopwatch.start();
+}
+ }
+
+ public void endTimer() {
+if(_executeStopwatch.isRunning()) {
+ _executeStopwatch.stop();
+}
+ }
+
+ public void recordInput(int numBlock, int numRows) {
+_numInputBlock += numBlock;
+_numInputRows += numRows;
+ }
+
+ public void recordOutput(int numBlock, int numRows) {
+_numOutputBlock += numBlock;
+_numOutputRows += numRows;
+ }
+
+ @Override
+ public String toString() {
Review Comment:
Added a TODO due to the time sensitivity. Do you have a recommended json
string format library?
--
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
[GitHub] [pinot] 61yao commented on a diff in pull request #10094: [multistage] [debuggability] OpChain and operator stats
61yao commented on code in PR #10094:
URL: https://github.com/apache/pinot/pull/10094#discussion_r1080712321
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SortOperator.java:
##
@@ -134,8 +150,10 @@ private void consumeInputBlocks() {
for (Object[] row : container) {
SelectionOperatorUtils.addToPriorityQueue(row, _rows,
_numRowsToKeep);
}
-
+_operatorStats.endTimer();
block = _upstreamOperator.nextBlock();
+_operatorStats.startTimer();
+_operatorStats.recordInput(1, block.getNumRows());
Review Comment:
Maybe block.getNumRows() is wrong. I updated it to container.size() and
verified it is working.
--
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
[GitHub] [pinot] vvivekiyer commented on pull request #10128: Support for DistinctSumMV and DistinctAvgMV aggregation functions
vvivekiyer commented on PR #10128: URL: https://github.com/apache/pinot/pull/10128#issuecomment-1396304548 Addressed review 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. 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
[GitHub] [pinot] walterddr commented on a diff in pull request #10144: Adding duration unit in trivy timeout
walterddr commented on code in PR #10144: URL: https://github.com/apache/pinot/pull/10144#discussion_r1080695552 ## .github/workflows/pinot_vuln_check.yml: ## @@ -19,18 +19,8 @@ name: Pinot Dependencies on: - push: -branches: - - master - pull_request: -branches: - - master -paths: - - "**/pom.xml" - - "**/package.json" - - "**/package-lock.json" - - "docker/images/pinot/**" - - ".github/workflows/**" + schedule: Review Comment: Why do we need a cron schedule? Let's still do triggered based? -- 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
[GitHub] [pinot] snleee commented on pull request #10144: Adding duration unit in trivy timeout
snleee commented on PR #10144: URL: https://github.com/apache/pinot/pull/10144#issuecomment-1396273909 I'm fine with the change but we will need to make one time effort to clean up the test. I think that may involve in bumping up multiple libraries. -- 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
[GitHub] [pinot] Jackie-Jiang merged pull request #10087: [Clean up] Remove the support for non-zero padding
Jackie-Jiang merged PR #10087: URL: https://github.com/apache/pinot/pull/10087 -- 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
[GitHub] [pinot] jtao15 commented on a diff in pull request #10136: Decouple dependency on using peer segment download scheme for doing metadata pushes to controller
jtao15 commented on code in PR #10136:
URL: https://github.com/apache/pinot/pull/10136#discussion_r1080680161
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactory.java:
##
@@ -46,25 +46,34 @@ public SegmentCommitterFactory(Logger segmentLogger,
ServerSegmentCompletionProt
_serverMetrics = serverMetrics;
}
+ /**
+ *
+ * @param isSplitCommit Indicates if the controller has enabled split commit
+ * @param params Parameters to use in the Segment completion request
+ * @param controllerVipUrl Unused,
+ * @return
+ * @throws URISyntaxException
+ */
public SegmentCommitter createSegmentCommitter(boolean isSplitCommit,
SegmentCompletionProtocol.Request.Params params,
String controllerVipUrl)
throws URISyntaxException {
if (!isSplitCommit) {
return new DefaultSegmentCommitter(_logger, _protocolHandler, params);
}
-SegmentUploader segmentUploader;
-// TODO Instead of using a peer segment download scheme to control how the
servers do split commit, we should use
-// other configs such as server or controller configs or controller
responses to the servers.
-if (_tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() !=
null) {
- segmentUploader = new
PinotFSSegmentUploader(_indexLoadingConfig.getSegmentStoreURI(),
- PinotFSSegmentUploader.DEFAULT_SEGMENT_UPLOAD_TIMEOUT_MILLIS);
- return new PeerSchemeSplitSegmentCommitter(_logger, _protocolHandler,
params, segmentUploader);
-}
-segmentUploader = new Server2ControllerSegmentUploader(_logger,
_protocolHandler.getFileUploadDownloadClient(),
+SegmentUploader segmentUploader = new
Server2ControllerSegmentUploader(_logger,
+_protocolHandler.getFileUploadDownloadClient(),
_protocolHandler.getSegmentCommitUploadURL(params, controllerVipUrl),
params.getSegmentName(),
ServerSegmentCompletionProtocolHandler.getSegmentUploadRequestTimeoutMs(),
_serverMetrics,
_protocolHandler.getAuthProvider());
-return new SplitSegmentCommitter(_logger, _protocolHandler, params,
segmentUploader);
+boolean uploadToFs =
_tableConfig.getValidationConfig().isUploadToFileSystem();
+String peerSegmentDownloadScheme =
_tableConfig.getValidationConfig().getPeerSegmentDownloadScheme();
+
+// TODO: exists for backwards compatibility. remove peerDownloadScheme
non-null check once users have migrated
+if (uploadToFs || peerSegmentDownloadScheme != null) {
Review Comment:
(nit) Move this condition before `L64` so `segmentUploader` won't be created
twice?
--
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
