Add isPartialResult flag to broker response #11592

saurabhd336 · 2023-09-14T14:03:18Z

A new flag to the broker response that explicitly denotes whether or not the result being returned is partial or not. This builds on top of PRs merged to resolve #7264

codecov-commenter · 2023-09-14T15:12:01Z

Codecov Report

Merging #11592 (08654b0) into master (025038a) will decrease coverage by 1.60%.
Report is 18 commits behind head on master.
The diff coverage is 62.50%.

@@             Coverage Diff              @@
##             master   #11592      +/-   ##
============================================
- Coverage     62.98%   61.39%   -1.60%     
+ Complexity     1141      207     -934     
============================================
  Files          2367     2373       +6     
  Lines        127958   128284     +326     
  Branches      19744    19804      +60     
============================================
- Hits          80593    78755    -1838     
- Misses        41630    43850    +2220     
+ Partials       5735     5679      -56

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.35% <62.50%> (-1.57%)`	⬇️
java-21	`61.25% <62.50%> (-1.58%)`	⬇️
skip-bytebuffers-false	`61.37% <62.50%> (-1.57%)`	⬇️
skip-bytebuffers-true	`61.24% <62.50%> (-1.57%)`	⬇️
temurin	`61.39% <62.50%> (-1.60%)`	⬇️
unittests	`61.38% <62.50%> (-1.60%)`	⬇️
unittests1	`46.67% <100.00%> (-20.33%)`	⬇️
unittests2	`27.55% <12.50%> (+13.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...t/common/response/broker/BrokerResponseNative.java	`94.02% <100.00%> (+0.13%)`	⬆️
...n/java/org/apache/pinot/client/ExecutionStats.java	`62.50% <50.00%> (-0.66%)`	⬇️
...roker/requesthandler/BaseBrokerRequestHandler.java	`46.00% <0.00%> (-0.10%)`	⬇️

... and 225 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

siddharthteotia · 2023-09-15T12:24:19Z

I thought this was already being returned as part of execution stats. I remember there was a flag hasPartialResults. How is this different ?

eaugene · 2023-09-15T18:03:33Z

...ker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java

+      brokerResponse.setPartialResult(true);
      brokerResponse.addToExceptions(new QueryProcessingException(QueryException.SERVER_SEGMENT_MISSING_ERROR_CODE,


If some servers are not available we currently used to throw an Exception instead of the results
So likely this won't be required as we have an exception stating which servers are in error

Jackie-Jiang

I don't see partial result being added when server returns exception. Did I miss it somewhere?

walterddr · 2023-09-21T19:13:05Z

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

@@ -607,6 +607,7 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S
      int numUnavailableSegments = unavailableSegments.size();
      requestContext.setNumUnavailableSegments(numUnavailableSegments);

+      boolean isPartialResult = false;


what is the "partial result" definition? for example if grouplimits reached for order by queries do we consider that as partial results?

i guess the question is when do we set the flag for:

result is correct (this is no flag)

result is correct but we didn't look at the entire table (e.g. select * limit 1000)

result is not correct but each row is correct (e.g. select ... group by key order by COUNT(*))

result is not correct, some rows might be correct some rows might not due to data size limit (select ... join ... group by ... when join hits maxRow limit, grouping result might be partial)

result is incorrect (we shouldn't return in this case)

@walterddr my idea with this flag is to generally cover all cases where we do sent a response back (no failures), but we also know either some segments / servers were not queried (segment unavailability / server down etc) or some rows / groups were ignored.

result is correct (this is no flag)

result is correct but we didn't look at the entire table (e.g. select * limit 1000) -> I don't think this should be flagged as partial result since the result is correct, we don't have to look the entire table up

result is not correct but each row is correct (e.g. select ... group by key order by COUNT(*)) -> I'm not sure if there is a way of knowing this in the current server -> broker response? If yes, then I do think we should flag this a partial result. I may not have full context, but is it the trimming of cross segment groups to max(limit * 5, DEFAULT_MIN_NUM_GROUPS) that you're referring to?

result is not correct, some rows might be correct some rows might not due to data size limit (select ... join ... group by ... when join hits maxRow limit, grouping result might be partial) -> Yes this should classify as partialResponse imo

result is incorrect (we shouldn't return in this case) -> What do you mean by incorrect here?

I think general definition (and this is what we did in the current engine IIRC) is that partialResults flag would be set to true in either of the following:

Result is incomplete (determined by server based on things like GROUP BY heuristics, threshold etc)

Not all servers queried responded (determine by broker)

Basically when we are able to determine that there was more data to be processed but we couldn't either because of our own algorithms (heuristics / thresholds etc) or failed to hear back from servers.

I think (1) can also be determined by broker during merge.

For SELECT with ORDER BY, I don't think we ever set partialResults on server and there is no need imo.

For SELECT without ORDER BY, servers do early termination based on LIMIT N, but again partialResults is not set and there is no need.

…Flag

saurabhd336 · 2023-10-16T11:05:49Z

@Jackie-Jiang @siddharthteotia @walterddr @eaugene
I've updated the PR as per the comments and some internal discussions
Partial response is defined as the case when pinot responds with a result, but is also aware that the result being returned has not considered all the data it should have. Cases that qualify as partial

Not all segments queried by the broker due to unavaliable segments / unavaliable servers
Not all servers responded (either no response or exceptions)
Other algorithm limitations like numGroupsLimitReached.

walterddr · 2023-10-16T15:15:34Z

...c/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java

      brokerResponse.addToExceptions(new QueryProcessingException(QueryException.SERVER_NOT_RESPONDING_ERROR_CODE,
          String.format("%d servers %s not responded", numServersNotResponded, serversNotResponded)));
      _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.BROKER_RESPONSES_WITH_PARTIAL_SERVERS_RESPONDED, 1);
    }
+    if (totalServersFailedOrNotResponded > 0) {


why do we need this? we can simply do

Suggested change

if (totalServersFailedOrNotResponded > 0) {

if (numServersQueried - numServersResponded > 0) {

if there's any exception inside the response, i thought the broker reducer is going to rectify and add proper partial result or failure inside?

for example if it is a SELECT * FROM tbl LIMIT 10 then

original logic: selection reducer will not tag partial response as long as there's 1 server returns more than 10 rows

in this PR: it will tag as long as 1 server returns an exception

is that the intention?

chatted with @Jackie-Jiang offline. i think the idea is to set this as "partial response" even when the results are completely correct. we still want to surface a global "flag" indicating that 'something can be improved underneath'.

@Jackie-Jiang @walterddr This was added to make sure any exceptions in the returned dataTable (even if the server has responded with a valid set of rows), are also flagged as potentially partial

This should not be handled here. We can simply check if (brokerResponse.getExceptionsSize() > 0) in the end in BaseBrokerRequestHandler and set the flag.

Jackie-Jiang · 2023-10-16T18:16:17Z

@Jackie-Jiang @siddharthteotia @walterddr @eaugene I've updated the PR as per the comments and some internal discussions Partial response is defined as the case when pinot responds with a result, but is also aware that the result being returned has not considered all the data it should have. Cases that qualify as partial

Not all segments queried by the broker due to unavaliable segments / unavaliable servers

Not all servers responded (either no response or exceptions)

Other algorithm limitations like numGroupsLimitReached.

@saurabhd336 We should count the result as partial if there are exceptions in the server response even if the server responded. One example would be when some segments dropped due to schema mismatch on the server side.

walterddr · 2023-10-18T23:48:25Z

i think there's just one situation when server returns no exception but we still want to set the partial results flag --> that's numGroupsLimitReach flag, which is only a boolean flag in the data table but not part of exception. if we can rectify this we should be good to only look at exceptions?

Jackie-Jiang · 2023-10-21T00:18:38Z

i think there's just one situation when server returns no exception but we still want to set the partial results flag --> that's numGroupsLimitReach flag, which is only a boolean flag in the data table but not part of exception. if we can rectify this we should be good to only look at exceptions?

We don't want to model it as an exception because it can happen in happy path, and some client might simply count query with exception as failure

saurabhd336 · 2023-10-23T06:58:40Z

@Jackie-Jiang as per the discussion above, does the PR look ok to you?

…Flag

Jackie-Jiang · 2023-10-25T17:41:26Z

...c/main/java/org/apache/pinot/broker/requesthandler/SingleConnectionBrokerRequestHandler.java

      brokerResponse.addToExceptions(new QueryProcessingException(QueryException.SERVER_NOT_RESPONDING_ERROR_CODE,
          String.format("%d servers %s not responded", numServersNotResponded, serversNotResponded)));
      _brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.BROKER_RESPONSES_WITH_PARTIAL_SERVERS_RESPONDED, 1);
    }
+    if (totalServersFailedOrNotResponded > 0) {


This should not be handled here. We can simply check if (brokerResponse.getExceptionsSize() > 0) in the end in BaseBrokerRequestHandler and set the flag.

Jackie-Jiang · 2023-10-25T17:42:22Z

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

+        // If segments are unavailable, the result should be considered partial
+        isPartialResult = true;


We don't need to update it here. We may set the value in the end by checking if the response contains exceptions

Add isPartialResult flag to broker response

c2b2f17

eaugene reviewed Sep 15, 2023

View reviewed changes

Jackie-Jiang added enhancement release-notes Referenced by PRs that need attention when compiling the next release notes labels Sep 19, 2023

Jackie-Jiang reviewed Sep 19, 2023

View reviewed changes

walterddr reviewed Sep 21, 2023

View reviewed changes

Saurabh Dubey added 2 commits October 16, 2023 15:36

Merge branch 'master' of github.com:apache/pinot into partialResponse…

7c3f3ce

…Flag

Set flag when server throws exceptions

8b0398b

walterddr reviewed Oct 16, 2023

View reviewed changes

saurabhd336 requested review from walterddr and Jackie-Jiang October 23, 2023 06:58

Merge branch 'master' of github.com:apache/pinot into partialResponse…

717ece2

…Flag

Jackie-Jiang reviewed Oct 25, 2023

View reviewed changes

saurabhd336 force-pushed the partialResponseFlag branch from dbff260 to c9010f5 Compare October 26, 2023 10:19

Review comments

08654b0

saurabhd336 force-pushed the partialResponseFlag branch from c9010f5 to 08654b0 Compare October 26, 2023 10:22

saurabhd336 requested a review from Jackie-Jiang October 26, 2023 10:22

Jackie-Jiang approved these changes Oct 30, 2023

View reviewed changes

Jackie-Jiang merged commit b4774c2 into apache:master Oct 30, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isPartialResult flag to broker response #11592

Add isPartialResult flag to broker response #11592

saurabhd336 commented Sep 14, 2023

codecov-commenter commented Sep 14, 2023 •

edited

Loading

siddharthteotia commented Sep 15, 2023

eaugene Sep 15, 2023

Jackie-Jiang left a comment

walterddr Sep 21, 2023 •

edited

Loading

saurabhd336 Oct 4, 2023

siddharthteotia Oct 4, 2023

saurabhd336 commented Oct 16, 2023

walterddr Oct 16, 2023 •

edited

Loading

walterddr Oct 16, 2023

saurabhd336 Oct 17, 2023

Jackie-Jiang Oct 25, 2023

Jackie-Jiang commented Oct 16, 2023

walterddr commented Oct 18, 2023

Jackie-Jiang commented Oct 21, 2023

saurabhd336 commented Oct 23, 2023

Jackie-Jiang Oct 25, 2023

Jackie-Jiang Oct 25, 2023

		brokerResponse.setPartialResult(true);
		brokerResponse.addToExceptions(new QueryProcessingException(QueryException.SERVER_SEGMENT_MISSING_ERROR_CODE,

	if (totalServersFailedOrNotResponded > 0) {
	if (numServersQueried - numServersResponded > 0) {

		// If segments are unavailable, the result should be considered partial
		isPartialResult = true;

Add isPartialResult flag to broker response #11592

Add isPartialResult flag to broker response #11592

Conversation

saurabhd336 commented Sep 14, 2023

codecov-commenter commented Sep 14, 2023 • edited Loading

Codecov Report

siddharthteotia commented Sep 15, 2023

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

walterddr Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhd336 commented Oct 16, 2023

walterddr Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang commented Oct 16, 2023

walterddr commented Oct 18, 2023

Jackie-Jiang commented Oct 21, 2023

saurabhd336 commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2023 •

edited

Loading

walterddr Sep 21, 2023 •

edited

Loading

walterddr Oct 16, 2023 •

edited

Loading