paul-rogers commented on PR #13071: URL: https://github.com/apache/druid/pull/13071#issuecomment-1265794737
@FrankChen021, thanks much for your detailed review! Very helpful. To answer your specific questions: 1. Sorry for the force-push. On many projects, the build is done on the PR itself. Any conflicts are resolved at the end of the review process. In Druid, each PR build is first rebased on master. Failure to rebase causes the build to fail and we get no feedback on the code. As a result, when there is a merge conflict, we are forced to rebase, which requires a force push. I wonder, is there some trick that others use to avoid this issue? 2. Sorry for the extra spaces in imports. That's just how Eclipse works. I'll make a note to manually remove such newlines in the future. 3. See below. 4. See below. Your point 3 suggests that callers always see `QueryContext` and point 4 worries about the cost of creating `QueryContext` per call. Looking more carefully at the prior implementation, there is a path to make this work. That fix appears in the latest commit. The result is we eliminate the copies that were your concern in point 4. Point 3 also suggests that callers always see `QueryContext` instead of the map. I agree that is the goal. The one caveat is that, for backward compatibility, the original map-based `getContext()` must exist and be a JSON property so that the JSON form of a query is unchanged. Thank you for pointing out the potential bug with updating a query context in place in a query. The general rule, after this change, should be: * The query context is created, as a map, before the query is constructed. * Once the query is constructed, the context is accessed via `QueryContext` and is immutable. (Again, the one exception is that the map is presented to Jackson for JSON serialization.) * If the context must change (such as while processing a native query), we must make a copy of the context then make a copy of the entire query with the new context. This ensures that the query is immutable and avoids the kinds of race conditions which first brought my attention to this area. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
