Allow ValueSet expansion into a discrete set for Bloom filtering

[rzeyde-varada]

Fixes #4108.

[findepi]

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

[rzeyde-varada]

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

[raunaqmorarka]

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]

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));
	}

[rzeyde-varada]

Many thanks for the review!

[rzeyde-varada]

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

[raunaqmorarka]

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]

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

Sounds good - this indeed simplifies 1edc70f significantly :)

[raunaqmorarka]

lgtm % comments

[rzeyde-varada]

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

[findepi]

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?

[rzeyde-varada]

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

[findepi]

remove static

[rzeyde-varada]

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

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

[findepi]

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

[rzeyde-varada]

You're right - my bad... 🤦

[rzeyde-varada]

Pushed a (hopefully) better version in 7a8425f088.

[findepi]

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

thanks!

[findepi]

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

[rzeyde-varada]

Thanks!
Fixed the CI :)

[findepi]

@sopel39 PTAL

[rzeyde-varada]

Simplified the code, following #9868 (comment).

[rzeyde-varada]

Thanks!