Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs commented on code in PR #24015:
URL: https://github.com/apache/flink/pull/24015#discussion_r1439142391
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/agg/AggsHandlerCodeGenerator.scala:
##
@@ -1230,8 +1232,26 @@ class AggsHandlerCodeGenerator(
needReset: Boolean = false,
needEmitValue: Boolean = false): Unit = {
// check and validate the needed methods
+aggBufferCodeGens.zipWithIndex.foreach {
+ case (aggBufferCodeGen, index) =>
+aggBufferCodeGen.checkNeededMethods(
+ needAccumulate,
+ needRetract && aggCallNeedRetractions(index),
+ needMerge,
Review Comment:
needRetract && index < aggCallNeedRetractions.length &&
aggCallNeedRetractions(index)
1. if needRetract is true, aggCallNeedRetractions can not be null, because
it will be set by needRetract(aggCallNeedRetractions: Array[Boolean]) and we
should add index < aggCallNeedRetractions.length because aggCalls have basic
functions , while aggBufferCodeGens have
countStar/distinct code function
3. if needRetract is false, although aggCallNeedRetractions is null, but
needRetract is false, will not run the logical of
aggCallNeedRetractions(index),. so it will not throw NPE or OutofBoundArray
Exception
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs commented on code in PR #24015:
URL: https://github.com/apache/flink/pull/24015#discussion_r1439142391
##
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/agg/AggsHandlerCodeGenerator.scala:
##
@@ -1230,8 +1232,26 @@ class AggsHandlerCodeGenerator(
needReset: Boolean = false,
needEmitValue: Boolean = false): Unit = {
// check and validate the needed methods
+aggBufferCodeGens.zipWithIndex.foreach {
+ case (aggBufferCodeGen, index) =>
+aggBufferCodeGen.checkNeededMethods(
+ needAccumulate,
+ needRetract && aggCallNeedRetractions(index),
+ needMerge,
Review Comment:
needRetract && aggCallNeedRetractions(index)
1. if needRetract is true, aggCallNeedRetractions can not be null, because
it will be set by needRetract(aggCallNeedRetractions: Array[Boolean])
2. if needRetract is false, although aggCallNeedRetractions is null, but
needRetract is false, will not run the logical of
aggCallNeedRetractions(index),. so it will not throw NPE or OutofBoundArray
Exception
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs commented on PR #24015: URL: https://github.com/apache/flink/pull/24015#issuecomment-1873602881 hi @xuyangzhong @lsyldliu i have add comments for important part, it will be convenient for review. will you have time to help 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs commented on code in PR #24015:
URL: https://github.com/apache/flink/pull/24015#discussion_r1439141361
##
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecLocalGroupAggregate.java:
##
@@ -147,7 +147,7 @@ protected Transformation translateToPlanInternal(
true);
generator.needAccumulate().needMerge(0, true, null);
if (needRetraction) {
-generator.needRetract();
+generator.needRetract(aggCallNeedRetractions);
}
Review Comment:
1. why global agg doesn't need retract method? because agg when enable two
stage. the global agg doesn't receive retract msg, so only local agg using
retract
2. why window WTF doesn't need retract method? because Window WTF can not
process update msg now. it will supports later
3. why group window agg doesn't need retract method? because the group
window agg doesn't support two stage in flink, but supports on blink
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
flinkbot commented on PR #24015: URL: https://github.com/apache/flink/pull/24015#issuecomment-1873601676 ## CI report: * c729879e94c5c441e661629a62a4b1653a73a045 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs commented on code in PR #24015: URL: https://github.com/apache/flink/pull/24015#discussion_r1439140313 ## flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/batch/BatchExecOverAggregate.java: ## @@ -231,7 +233,7 @@ private List createOverWindowFrames( GeneratedAggsHandleFunction genAggsHandler = generator .needAccumulate() -.needRetract() +.needRetract(aggCallNeedRetractions) Review Comment: comments : we may not use retract in batch sql, but i found OffsetOverFrame(belongs to BatchExecOverAggregate) use retract function -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-33951][table] Use aggCallNeedRetractions instead of needRetrac… [flink]
liuyongvs opened a new pull request, #24015: URL: https://github.com/apache/flink/pull/24015 ## What is the purpose of the change * should use aggCallNeedRetractions instead of needRetraction to check retract method, orelse throw not implement retract method* ## Brief change log * should use aggCallNeedRetractions instead of needRetraction to check retract method, orelse throw not implement retract method* ## Verifying this change * original unit test * ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no ) - The runtime per-record code paths (performance sensitive): (no ) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
