Skip to content

Add interface method for returning canonical lookup name #16557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abhishekrb19 merged 4 commits into from
Jun 5, 2024
Merged

Add interface method for returning canonical lookup name #16557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5b4fa4
Add interface method for returning canonical lookup name
Akshat-Jain Jun 5, 2024
ee9d0e0
Address review comment
Akshat-Jain Jun 5, 2024
2a255ee
Add test in LookupReferencesManagerTest for coverage check
Akshat-Jain Jun 5, 2024
42417b9
Add test in LookupSerdeModuleTest for coverage check
Akshat-Jain Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions benchmarks/src/test/java/org/apache/druid/benchmark/JoinAndLookupBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
return Optional.empty();
}
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}
)
)
Expand Down
5 changes: 5 additions & 0 deletions .../src/main/java/org/apache/druid/query/lookup/LookupExtractorFactoryContainerProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,9 @@ public interface LookupExtractorFactoryContainerProvider
* Returns a lookup container for the provided lookupName, if it exists.
*/
Optional<LookupExtractorFactoryContainer> get(String lookupName);

/**
* Returns the canonical lookup name from a given lookup name, if special syntax exists.
*/
String getCanonicalLookupName(String lookupName);
Copy link
Copy Markdown
Contributor

@LakshSingla LakshSingla Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming nit:

  1. 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:

  1. This method isn't marked as PublicApi however custom extensions can extend it for own lookup implementations. Please add a default implementation for the same.
  2. 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?

Copy link
Copy Markdown
Contributor Author

@Akshat-Jain Akshat-Jain Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LakshSingla
I had a discussion with @abhishekrb19 about the naming of this method. We concluded on getCanonicalLookupName().

Regarding the other comments:

  1. I don't see the other methods marked as PublicApi either? So seems like an unrelated issue to this PR? Thoughts?
  2. 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.

Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
6 changes: 6 additions & 0 deletions processing/src/test/java/org/apache/druid/query/expression/LookupExprMacroTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
{
return Optional.empty();
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
})
);
}
Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,12 @@ public Set<String> getAllLookupNames()
return stateRef.get().lookupMap.keySet();
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}

// Note that this should ensure that "toLoad" and "toDrop" are disjoint.
LookupsState<LookupExtractorFactoryContainer> getAllLookupsState()
{
Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/apache/druid/query/lookup/LookupSerdeModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,11 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
{
return Optional.empty();
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}
}
6 changes: 6 additions & 0 deletions server/src/test/java/org/apache/druid/query/expression/LookupEnabledTestExprMacroTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
return Optional.empty();
}
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
};
}

Expand Down
7 changes: 7 additions & 0 deletions server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,13 @@ public void testGetAllLookupNames() throws Exception
);
}

@Test
public void testGetCanonicalLookupName()
{
String lookupName = "lookupName1";
Assert.assertEquals(lookupName, lookupReferencesManager.getCanonicalLookupName(lookupName));
}

@Test
public void testGetAllLookupsState() throws Exception
{
Expand Down
8 changes: 8 additions & 0 deletions server/src/test/java/org/apache/druid/query/lookup/LookupSerdeModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,12 @@ public void testExpressionTransformSerde() throws Exception
objectMapper.readValue(objectMapper.writeValueAsBytes(transform), ExpressionTransform.class)
);
}

@Test
public void testGetCanonicalLookupName()
{
LookupExtractorFactoryContainerProvider instance = injector.getInstance(LookupExtractorFactoryContainerProvider.class);
String lookupName = "lookupName1";
Assert.assertEquals(lookupName, instance.getCanonicalLookupName(lookupName));
}
}
6 changes: 6 additions & 0 deletions server/src/test/java/org/apache/druid/segment/LookupSegmentWranglerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ public Optional<LookupExtractorFactoryContainer> get(final String lookupName)
return Optional.empty();
}
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}
);

Expand Down
6 changes: 6 additions & 0 deletions server/src/test/java/org/apache/druid/segment/join/LookupJoinableFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
return Optional.empty();
}
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}
);
}
Expand Down
6 changes: 6 additions & 0 deletions server/src/test/java/org/apache/druid/server/SpecificSegmentsQuerySegmentWalker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
{
return Optional.empty();
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
};

public static SpecificSegmentsQuerySegmentWalker createWalker(
Expand Down
6 changes: 6 additions & 0 deletions services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,12 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
{
return Optional.empty();
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}

final TimeseriesQuery query =
Expand Down
2 changes: 1 addition & 1 deletion ...n/java/org/apache/druid/sql/calcite/expression/builtin/QueryLookupOperatorConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public DruidExpression toDruidExpression(
final String lookupName = (String) lookupNameExpr.getLiteralValue();

// Add the lookup name to the set of lookups to selectively load.
plannerContext.addLookupToLoad(lookupName);
plannerContext.addLookupToLoad(lookupExtractorFactoryContainerProvider.getCanonicalLookupName(lookupName));

if (arg.isSimpleExtraction() && lookupNameExpr.isLiteral()) {
return arg.getSimpleExtraction().cascade(
Expand Down
6 changes: 6 additions & 0 deletions sql/src/test/java/org/apache/druid/sql/calcite/util/TestLookupProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,10 @@ public Optional<LookupExtractorFactoryContainer> get(String lookupName)
return Optional.empty();
}
}

@Override
public String getCanonicalLookupName(String lookupName)
{
return lookupName;
}
}