Trino - Optimize visibility filtering using Iceberg metadata

[andresdantereng]

This optimizes filtering based on visibility values by filtering Iceberg files based on visibility min-max values in metadata. Two new properties are required:

• geomesa.security.enable-visibility-expression-pruning: False by default. This gates the entire feature. If set to true, it applies the optimization.
• geomesa.security.visibility-expressions: A comma-separated list of possible visibility expressions, such as FOO,FOO&BAR.

closes #10818

[cwdobbins]

docs/user/trino/security.rst needs the new flag documented

[cwdobbins]

IcebergVisibilityPruningMechanismTest's tests cover only emptyAuthsDomain and tokenDomain. There is no expressionDomain case at all. Worth adding:

• A file with __vis__ = "admin&ops" survives under expressionDomain({"admin&ops"}, {"admin","ops"}) where tokenDomain prunes it. This is the entire justification for the PR and is currently only asserted at the Domain level (expressionDomainFixesTokenDomainsCompoundExpressionGap), never through the manifest evaluator.

[andresdantereng]

This is done.

[cwdobbins]

Edge case (unlikely to occur in prod, but should be fixed):

The isVisible UDF treats empty vis entries ("" \ length() == 0) as unrestricted, so the code anticipates "" occurring in the column. But parseVisibilityExpressions drops blank entries, so "" can never be declared; the domain admits NULL only. If any row carries "" rather than NULL, files holding only empty __vis__ entries are pruned rather than returned.

[elahrvivaz]

could you add the new properties to docs/user/trino/security.rst ? (they are connector properties, i think?)

[elahrvivaz]

Edge case (unlikely to occur in prod, but should be fixed):

The isVisible UDF treats empty vis entries ("" \ length() == 0) as unrestricted, so the code anticipates "" occurring in the column. But parseVisibilityExpressions drops blank entries, so "" can never be declared; the domain admits NULL only. If any row carries "" rather than NULL, files holding only empty __vis__ entries are pruned rather than returned.

If it simplifies things, I think we could just assume that if vis are being used, then no vis are empty/null - in practice, if vis are enabled then it is likely to be an error if a vis is empty/null. in other stores, we actually have a sys prop to enforce setting vis at write time - we may want to add that to trino as well.

[andresdantereng]

docs/user/trino/security.rst needs the new flag documented

This is also done, but let me know if I added too much documentation.

[andresdantereng]

could you add the new properties to docs/user/trino/security.rst ? (they are connector properties, i think?)

Done

[cwdobbins]

Edge case (unlikely to occur in prod, but should be fixed):
The isVisible UDF treats empty vis entries ("" \ length() == 0) as unrestricted, so the code anticipates "" occurring in the column. But parseVisibilityExpressions drops blank entries, so "" can never be declared; the domain admits NULL only. If any row carries "" rather than NULL, files holding only empty __vis__ entries are pruned rather than returned.

If it simplifies things, I think we could just assume that if vis are being used, then no vis are empty/null - in practice, if vis are enabled then it is likely to be an error if a vis is empty/null. in other stores, we actually have a sys prop to enforce setting vis at write time - we may want to add that to trino as well.

Ok, after offline discussion, we are going to fail-closed on rows with null/empty vis, when a vis column is present and visibility filtering is flagged as enabled, so we would 1) return no rows lacking an empty vis, and thus 2) prune out any files whose contents contain solely empty/null vis

[elahrvivaz]

this is ok for now, but we should prefer using trino through testcontainers instead of embedded test frameworks, for the very reason that embedded frameworks complicate the classpath and don't always accurately reflect a real instance. (we have an example in TrinoDataStoreTest of using testcontainers)

[elahrvivaz]

actually, I didn't realize the test framework was being added in this PR - I think we should switch to using testcontainers instead.

[elahrvivaz]

you could move the IT into the trino data store module, or otherwise you'll just need to make sure that the plugin is packaged and available to load into the trino image when the tests run - i.e. just make sure the maven lifecycle is correct.

[andresdantereng]

Sounds like moving the IT into the Trino datastore module is the right thing to do. I definitely chose the wrong path on this one. The changes have been pushed, but I'm seeing errors when I run it locally, so I'm not out of the woods on this one yet. I'll update when I have dealt with all the issues.

[andresdantereng]

This should be okay now. I was consistently able to run this:

mvn -pl geomesa-trino/geomesa-trino-datastore -am verify \
-DskipITs=false \
-Dit.test=VisibilityPruningEndToEndIT \
-Dtest=NONE \
-Dsurefire.failIfNoSpecifiedTests=false \
-Dfailsafe.failIfNoSpecifiedTests=false

[elahrvivaz]

looks like there are test failures in CI, also