Add interface method for returning canonical lookup name#16557
Add interface method for returning canonical lookup name#16557abhishekrb19 merged 4 commits intoapache:masterfrom
Conversation
abhishekrb19
left a comment
There was a problem hiding this comment.
Thanks, @Akshat-Jain!
...ing/src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Returns the canonical lookup name from a lookup name. | ||
| */ | ||
| String getCanonicalLookupName(String lookupName); |
There was a problem hiding this comment.
naming nit:
- This can be getLookupName(String name). Canonical is confusing, and isn't defined in the javadoc here. See point (2) below as well.
other comments:
- This method isn't marked as
PublicApihowever custom extensions can extend it for own lookup implementations. Please add a default implementation for the same. - When should this method be called. As of now, this is a super specialized method that is called in
QueryLookupOperatorConversion. Perhaps it should be called out and named as such. As a developer, it is unclear to me when must I call it and when not. For example, why am I not calling it on https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/schema/LookupSchema.java#L61?
There was a problem hiding this comment.
@LakshSingla
I had a discussion with @abhishekrb19 about the naming of this method. We concluded on getCanonicalLookupName().
Regarding the other comments:
- I don't see the other methods marked as PublicApi either? So seems like an unrelated issue to this PR? Thoughts?
- This would be used in a custom extension. Would raise a future PR once this merges where it would be more clear. But in general, if in future we add a lookup name with special syntaxing, then this would be needed.
There was a problem hiding this comment.
@LakshSingla, good points. I think we need to clarify the Javadoc a bit more to clarify the intent of the new method and/or rename accordingly. We can also call it getLookupName(String name) too, but that's a bit ambiguous imo. Naming, thoughts? 😅
As far as 1 is concerned, the interface isn't annotated @PublicAPI or @ExtensionsPoint. Also, this PR #9281 changed the method signature and added a new interface method without default implementations. Following that pattern, I believe it's safe to add new methods without providing a default implementation. In general, my understanding is that for custom extensions that directly use/extend interfaces that are not public APIs (i.e., not annotated with PublicAPI or ExtensionsPoint), the onus is on the developers maintaining custom implementations to sync with changes coming from upstream.
For 2, LookupSchema is powering the SQL view for the lookups configured in the system. For consistency, I think it'd make sense to also call the new function there as you pointed out. I'm okay with doing that as a follow up too.
|
Merging this PR. The two suggestions from @LakshSingla can be addressed in a follow-up. Thanks for the fix, @Akshat-Jain! |
Problem
Custom lookup extensions can have different lookup name syntax - i.e., the
LOOKUPfunction can have semantics for the lookup name. With the selective loading of lookups in MSQ, the lookup loads only the required lookups based on what's specified in the ingest SQL.However, with custom lookup implementations, the
lookupNameparsed from the query can differ from what's configured. Because of this mismatch, theLookupReferencesManagerwill not load the lookups specified in the query if it doesn't find an exact match with what's configured with the coordinator. As a result, MSQ queries involving such lookups will fail.Description
This PR adds support for returning the canonical lookup name in the
LookupExtractorFactoryContainerProviderinterface. It delegates getting the sanitized lookup name to the underlyingLookupExtractorFactoryContainerProvider:druid-lookups-cached-global,druid-lookups-cached-singleandkafka-extraction-namespace, the canonical name is the same as the lookup name.This PR has: