paul-rogers commented on PR #13022: URL: https://github.com/apache/druid/pull/13022#issuecomment-1242936186
@FrankChen021, your goal of type-safe conversion is a good one. The question is how to achieve that goal so that you can get this PR done, while also allowing us to solve the other issues in the other PR. Perhaps the most expedient solution is the one you suggest: go ahead and commit this PR. The other one will undo the new `Query` methods; but that should be OK because they won't have been around long enough for extensions to use them. Replacing calls to your new methods with the other approach should just be a simple text replace operation, which I can do in the other PR. The question we're asking is: should we add more methods to the `Query` interface, or should we move those responsibilities to a new class? In this PR, we've added more `getContextX` methods to `Query`. In the other PR, we've actually deprecated the existing ones in favor of the new `QueryContext` facade. If we want to consider alternatives, let's note that if we commit this one first, then the other PR (assuming it is approved), would immediately remove the new `getContextX` methods: they'd live only for the time between the two commits. So, perhaps an alternative solution is to use the existing `QueryContexts` methods in this PR, then switch to the facade in the next PR. For example, consider this line in this PR: ```java final String timestampStringFromContext = query.getContextAsString(CTX_KEY_FUDGE_TIMESTAMP, ""); ``` With the suggested "temporary" solution, perhaps write this as: ```java final String timestampStringFromContext = QueryContexts.getAsString(query, CTX_KEY_FUDGE_TIMESTAMP, ""); ``` In the later PR, this would become: ```java final String timestampStringFromContext = query.queryContext().getString(CTX_KEY_FUDGE_TIMESTAMP, ""); ``` The alternative approach avoids adding new methods to the `Query` interface. Otherwise, the two approaches are basically the same. There is one other difference, however: all the `getFoo()` methods in `QueryContexts`. With the approach in this PR, we'd have two ways to get values: ```java final String timestampStringFromContext = query.getContextAsString(CTX_KEY_FUDGE_TIMESTAMP, ""); final boolean flag = QueryContexts.getEnableJoinFilterRewrite(query); ``` In the other PR, we can move all the getters to a single class: ```java final QueryContext queryContext = query.queryContext(); final String timestampStringFromContext = queryContext.getString(CTX_KEY_FUDGE_TIMESTAMP, ""); final boolean flag = queryContext.enableJoinFilterRewrite(); ``` The result is simpler code, while providing type safety, concurrency safety, and the ability to authorize context values. So, the only question is how we help you get this one done now, and how we move onto the broader fix a bit later. Thoughts? -- 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]
