Allow ValueSet expansion into a discrete set for Bloom filtering #9868

rzeyde-varada · 2021-11-04T12:17:39Z

Fixes #4108.

core/trino-spi/src/main/java/io/trino/spi/type/BigintType.java

core/trino-spi/src/main/java/io/trino/spi/type/Type.java

core/trino-spi/src/main/java/io/trino/spi/predicate/Utils.java

lib/trino-orc/src/main/java/io/trino/orc/OrcReaderOptions.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OrcReaderConfig.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveSessionProperties.java

core/trino-spi/src/main/java/io/trino/spi/predicate/Utils.java

findepi · 2021-11-04T14:14:11Z

Wonder whether user(s) of io.trino.spi.predicate.TupleDomain#extractDiscreteValues would benefit from a similar range unpacking.

rzeyde-varada · 2021-11-07T16:02:57Z

Wonder whether user(s) of io.trino.spi.predicate.TupleDomain#extractDiscreteValues would benefit from a similar range unpacking.

IIUC, it has a single usage here:

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveBucketing.java

Line 230 in 8f5aa1c

    
           Optional<Map<ColumnHandle, List<NullableValue>>> bindings = TupleDomain.extractDiscreteValues(effectivePredicate);

I think it should be OK to try range expansion here as well, but we need to apply a reasonable limit - maybe reuse/rename getOrcBloomFiltersExpandDiscreteValuesLimit(session)?
If it's too complex, we can do it in a separate PR...

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java

raunaqmorarka · 2021-11-08T16:58:10Z

Wonder whether user(s) of io.trino.spi.predicate.TupleDomain#extractDiscreteValues would benefit from a similar range unpacking.

IIUC, it has a single usage here:

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveBucketing.java

Line 230 in 8f5aa1c

Optional<Map<ColumnHandle, List<NullableValue>>> bindings = TupleDomain.extractDiscreteValues(effectivePredicate);

I think it should be OK to try range expansion here as well, but we need to apply a reasonable limit - maybe reuse/rename getOrcBloomFiltersExpandDiscreteValuesLimit(session)? If it's too complex, we can do it in a separate PR...

In the current code, we use getHiveBucketFilter before we call simplify on tuple domain. So it would not be beneficial unless there is some other way to encounter a compacted TupleDomain there

rzeyde-varada · 2021-11-10T08:21:37Z

IIUC, it may help if we use an inequality predicate (e.g. col >= 1 AND col <= 5) over a bucketing column here:

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

Lines 2501 to 2517 in 1b5db4a

    
           public Optional<ConstraintApplicationResult<ConnectorTableHandle>> applyFilter(ConnectorSession session, ConnectorTableHandle tableHandle, Constraint constraint) 
        
           { 
        
               HiveTableHandle handle = (HiveTableHandle) tableHandle; 
        
               checkArgument(handle.getAnalyzePartitionValues().isEmpty() || constraint.getSummary().isAll(), "Analyze should not have a constraint"); 
        
               HivePartitionResult partitionResult = partitionManager.getPartitions(metastore, new HiveIdentity(session), handle, constraint); 
        
               HiveTableHandle newHandle = partitionManager.applyPartitionResult(handle, partitionResult, constraint.getPredicateColumns()); 
        
               if (handle.getPartitions().equals(newHandle.getPartitions()) && 
        
                       handle.getCompactEffectivePredicate().equals(newHandle.getCompactEffectivePredicate()) && 
        
                       handle.getBucketFilter().equals(newHandle.getBucketFilter()) && 
        
                       handle.getConstraintColumns().equals(newHandle.getConstraintColumns())) { 
        
                   return Optional.empty(); 
        
               } 
        
               return Optional.of(new ConstraintApplicationResult<>(newHandle, partitionResult.getUnenforcedConstraint(), false)); 
        
           }

core/trino-spi/src/main/java/io/trino/spi/type/BigintType.java

core/trino-spi/src/test/java/io/trino/spi/predicate/TestValueSet.java

core/trino-spi/src/main/java/io/trino/spi/type/Type.java

lib/trino-orc/src/main/java/io/trino/orc/OrcReaderOptions.java

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java

rzeyde-varada · 2021-11-16T11:33:51Z

Many thanks for the review!

rzeyde-varada · 2021-11-16T12:20:41Z

I changed DEFAULT_EXPAND_DISCRETE_VALUES_LIMIT to be equal to DEFAULT_COMPACTION_THRESHOLD, following #9868 (comment).

raunaqmorarka · 2021-11-16T14:29:32Z

I changed DEFAULT_EXPAND_DISCRETE_VALUES_LIMIT to be equal to DEFAULT_COMPACTION_THRESHOLD, following #9868 (comment).

Can we remove the new config altogether and just derive the limit from existing compaction threshold config ?

rzeyde-varada · 2021-11-16T17:30:46Z

Can we remove the new config altogether and just derive the limit from existing compaction threshold config ?

Sounds good - this indeed simplifies 1edc70f significantly :)

core/trino-spi/src/main/java/io/trino/spi/predicate/Utils.java

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java

raunaqmorarka

lgtm % comments

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java

lib/trino-orc/src/test/java/io/trino/orc/TestOrcBloomFilters.java

rzeyde-varada · 2022-01-08T06:26:57Z

Thanks for the review and the approval!
Updated the PR and rebased.

findepi · 2022-01-10T08:43:50Z

rzeyde-varada force-pushed the expand-ranges branch from 7e204a1 to 94a1a1d 3 days ago

ouch, rebase. this gives me https://github.com/trinodb/trino/compare/7e204a1545ea3b739148cd2036fb6a428c2f4c69..94a1a1d247c188dd6deea2ac61bbc31575908f73 as the diff. Has anything changed there?

Also, implement it for basic integral types. Following trinodb#9868 (comment)

rzeyde-varada · 2022-01-10T21:50:35Z

Oops.... un-rebased, so the following diff can be reviewed more easily:
https://github.com/trinodb/trino/compare/7e204a1545ea3b739148cd2036fb6a428c2f4c69..cafcdda533be11effecf7fdda350102957711d78

findepi · 2022-01-11T08:58:19Z

core/trino-spi/src/main/java/io/trino/spi/predicate/ValueSet.java

+     * For example: [1, 5] can be expanded into {1, 2, 3, 4, 5}.
+     * If the data type is not supported or the expansion results in too many values, {@code Optional.empty()} is returned.
+     */
+    static Optional<Collection<Object>> tryExpandRanges(ValueSet valuesSet, int valuesLimit)


remove static

Not sure - if I remove it, the build will fail (IIUC):

[ERROR] .../ValueSet.java:[176,5] interface abstract methods cannot have body

The operation takes ValueSet and is defined in ValueSet. Why not instance/interface method?

You're right - my bad... 🤦

Pushed a (hopefully) better version in 7a8425f088.

findepi · 2022-01-11T08:58:35Z

Oops.... un-rebased, so the following diff can be reviewed more easily:

thanks!

findepi · 2022-01-15T11:33:26Z

@rzeyde-varada thanks for the update. Please see CI red.

rzeyde-varada · 2022-01-16T08:22:03Z

Thanks!
Fixed the CI :)

findepi

@sopel39 PTAL

rzeyde-varada · 2022-01-18T09:22:08Z

Simplified the code, following #9868 (comment).

Fixes trinodb#4108.

Also, implement it for basic integral types. Following #9868 (comment)

rzeyde-varada · 2022-01-21T13:46:03Z

Thanks!

cla-bot bot added the cla-signed label Nov 4, 2021

rzeyde-varada requested review from sopel39 and raunaqmorarka November 4, 2021 12:18

rzeyde-varada mentioned this pull request Nov 4, 2021

Allow merging consecutive 'long' values in SortedRangeSet #3316

Closed

rzeyde-varada force-pushed the expand-ranges branch from e576072 to 937bdf5 Compare November 4, 2021 12:56

findepi reviewed Nov 4, 2021

View reviewed changes

rzeyde-varada force-pushed the expand-ranges branch from 937bdf5 to d425c2c Compare November 5, 2021 12:43

raunaqmorarka reviewed Nov 8, 2021

View reviewed changes

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java Outdated Show resolved Hide resolved

raunaqmorarka reviewed Nov 10, 2021

View reviewed changes

rzeyde-varada force-pushed the expand-ranges branch 3 times, most recently from c9fd6b7 to 27a756c Compare November 16, 2021 11:33

rzeyde-varada requested review from raunaqmorarka and findepi November 16, 2021 11:33

rzeyde-varada force-pushed the expand-ranges branch from 27a756c to 1edc70f Compare November 16, 2021 17:27

raunaqmorarka reviewed Nov 16, 2021

View reviewed changes

rzeyde-varada force-pushed the expand-ranges branch 2 times, most recently from b69aeb1 to 311a105 Compare November 18, 2021 12:18

rzeyde-varada requested a review from raunaqmorarka November 18, 2021 12:19

raunaqmorarka reviewed Nov 18, 2021

View reviewed changes

lib/trino-orc/src/main/java/io/trino/orc/TupleDomainOrcPredicate.java Outdated Show resolved Hide resolved

lib/trino-orc/src/test/java/io/trino/orc/TestOrcBloomFilters.java Show resolved Hide resolved

rzeyde-varada force-pushed the expand-ranges branch from 7e204a1 to 94a1a1d Compare January 7, 2022 20:27

rzeyde-varada force-pushed the expand-ranges branch from 94a1a1d to 70c220b Compare January 8, 2022 06:20

rzeyde-varada requested a review from findepi January 8, 2022 08:50

rzeyde-varada added 2 commits January 10, 2022 23:46

Fix a small comment typo

87c16ae

Add getDiscreteValues(Range) to Type

6449d9a

Also, implement it for basic integral types. Following trinodb#9868 (comment)

rzeyde-varada force-pushed the expand-ranges branch from 70c220b to cafcdda Compare January 10, 2022 21:49

findepi reviewed Jan 11, 2022

View reviewed changes

rzeyde-varada requested a review from findepi January 11, 2022 17:46

rzeyde-varada force-pushed the expand-ranges branch from cafcdda to 7a8425f Compare January 14, 2022 15:58

github-actions bot added the tests:hive label Jan 14, 2022

rzeyde-varada force-pushed the expand-ranges branch from 7a8425f to 112fb76 Compare January 16, 2022 06:16

findepi approved these changes Jan 16, 2022

View reviewed changes

rzeyde-varada force-pushed the expand-ranges branch from 112fb76 to db12517 Compare January 18, 2022 09:21

Allow ValueSet expansion into a discrete set for Bloom filtering

f96ea0f

Fixes trinodb#4108.

rzeyde-varada force-pushed the expand-ranges branch from db12517 to f96ea0f Compare January 19, 2022 06:36

findepi merged commit 3208171 into trinodb:master Jan 20, 2022

findepi pushed a commit that referenced this pull request Jan 20, 2022

Add getDiscreteValues(Range) to Type

6e4dbc8

Also, implement it for basic integral types. Following #9868 (comment)

findepi mentioned this pull request Jan 20, 2022

Release notes for 369 #10552

Closed

github-actions bot added this to the 369 milestone Jan 20, 2022

mosabua mentioned this pull request Jan 20, 2022

Add Trino 369 release notes #10553

Merged

rzeyde-varada deleted the expand-ranges branch January 21, 2022 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ValueSet expansion into a discrete set for Bloom filtering #9868

Allow ValueSet expansion into a discrete set for Bloom filtering #9868

rzeyde-varada commented Nov 4, 2021

findepi commented Nov 4, 2021

rzeyde-varada commented Nov 7, 2021

raunaqmorarka commented Nov 8, 2021

rzeyde-varada commented Nov 10, 2021

rzeyde-varada commented Nov 16, 2021

rzeyde-varada commented Nov 16, 2021

raunaqmorarka commented Nov 16, 2021

rzeyde-varada commented Nov 16, 2021

raunaqmorarka left a comment

rzeyde-varada commented Jan 8, 2022

findepi commented Jan 10, 2022

rzeyde-varada commented Jan 10, 2022

findepi Jan 11, 2022

rzeyde-varada Jan 11, 2022

findepi Jan 12, 2022

rzeyde-varada Jan 14, 2022 •

edited

Loading

rzeyde-varada Jan 14, 2022

findepi commented Jan 11, 2022

findepi commented Jan 15, 2022

rzeyde-varada commented Jan 16, 2022

findepi left a comment

rzeyde-varada commented Jan 18, 2022

rzeyde-varada commented Jan 21, 2022

Allow ValueSet expansion into a discrete set for Bloom filtering #9868

Allow ValueSet expansion into a discrete set for Bloom filtering #9868

Conversation

rzeyde-varada commented Nov 4, 2021

findepi commented Nov 4, 2021

rzeyde-varada commented Nov 7, 2021

raunaqmorarka commented Nov 8, 2021

rzeyde-varada commented Nov 10, 2021

rzeyde-varada commented Nov 16, 2021

rzeyde-varada commented Nov 16, 2021

raunaqmorarka commented Nov 16, 2021

rzeyde-varada commented Nov 16, 2021

raunaqmorarka left a comment

Choose a reason for hiding this comment

rzeyde-varada commented Jan 8, 2022

findepi commented Jan 10, 2022

rzeyde-varada commented Jan 10, 2022

findepi Jan 11, 2022

Choose a reason for hiding this comment

rzeyde-varada Jan 11, 2022

Choose a reason for hiding this comment

findepi Jan 12, 2022

Choose a reason for hiding this comment

rzeyde-varada Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

rzeyde-varada Jan 14, 2022

Choose a reason for hiding this comment

findepi commented Jan 11, 2022

findepi commented Jan 15, 2022

rzeyde-varada commented Jan 16, 2022

findepi left a comment

Choose a reason for hiding this comment

rzeyde-varada commented Jan 18, 2022

rzeyde-varada commented Jan 21, 2022

rzeyde-varada Jan 14, 2022 •

edited

Loading