Report columns covered by a Dynamic Filter #9644
Conversation
findepi
force-pushed
the
findepi/introspectible-df
branch
from
edd52b0 to
e16a0ae
Compare
|
|
||
| Set<ColumnHandle> columnsCovered = descriptors.stream() | ||
| .map(Descriptor::getInput) | ||
| .map(SymbolsExtractor::extractUnique) // TODO can input be something else than SymbolReference? future's code above assumes not |
There was a problem hiding this comment.
I think we need to use DynamicFilters#extractSourceSymbol
There was a problem hiding this comment.
i will use extractSourceSymbol in the code here, thanks.
but is the code above (preexisting) correct?
There was a problem hiding this comment.
Existing code is relying extractSourceSymbols which uses DynamicFilters#extractSourceSymbol internally. RemoveUnsupportedDynamicFilters#isSupportedDynamicFilterExpression should ensure that the condition there is satisfied.
There was a problem hiding this comment.
but is the code above (preexisting) correct?
Yes. The code above operates on descriptorMap = extractSourceSymbols(descriptors) which unwraps cast
There was a problem hiding this comment.
i wouldn't find this myself, thanks. to me it seems like extractSourceSymbols creates a "fake" Descriptor.
|
|
||
| Set<ColumnHandle> columnsCovered = descriptors.stream() | ||
| .map(Descriptor::getInput) | ||
| .map(SymbolsExtractor::extractUnique) // TODO can input be something else than SymbolReference? future's code above assumes not |
There was a problem hiding this comment.
but is the code above (preexisting) correct?
Yes. The code above operates on descriptorMap = extractSourceSymbols(descriptors) which unwraps cast
| @@ -476,6 +503,8 @@ public void close() | |||
| @Override | |||
| public boolean isFinished() | |||
| { | |||
| assertEquals(dynamicFilter.getColumnsCovered(), expectedDynamicFilterColumnsCovered, "columns covered"); | |||
There was a problem hiding this comment.
the check in constructor should is sufficient
There was a problem hiding this comment.
i want to test that getColumnsCovered remains correct.
| @@ -514,6 +545,8 @@ public ConnectorPageSource createPageSource( | |||
| @Override | |||
| public boolean isFinished() | |||
| { | |||
| assertEquals(dynamicFilter.getColumnsCovered(), expectedDynamicFilterColumnsCovered, "columns covered"); | |||
There was a problem hiding this comment.
check in constructor should is sufficient
There was a problem hiding this comment.
i want to test that getColumnsCovered remains correct.
| @Override | ||
| public Set<ColumnHandle> getColumnsCovered() | ||
| { | ||
| return tupleDomain.getDomains().map(Map::keySet) |
There was a problem hiding this comment.
@sopel39 what do you mean?
There was a problem hiding this comment.
why not return empty array here
There was a problem hiding this comment.
we're given a tupleDomain here, so returning empty set would not be correct, would it be?
findepi
force-pushed
the
findepi/introspectible-df
branch
from
8076368 to
10c1c38
Compare
This allows a connector to make decision whether to wait for a filter or not.
findepi
force-pushed
the
findepi/introspectible-df
branch
from
10c1c38 to
89761e8
Compare
This allows a connector to make decision whether to wait for a filter or
not.