Support Metrics mode when creating/writing Iceberg tables

[liqinrae]

Added support for all metrics modes in orc format as mentioned in #9791

1. Default metrics mode is changed to truncate(16)
2. Allow for configuring default mode to NONE so that we can skip all metrics by default except the ones we want
3. Allows per column metrics mode configuration to support different modes: counts, none, truncate(length) and full

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: q-li.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[rdsr]

Do we not handle metrics mode truncate?

[liqinrae]

@rdsr yes we do, we handle truncate in the IcebergMinMax

[rdsr]

I see we added support for this below

[jackye1995]

the whole table property map is already passed in as a part of the writable table handle, why do we need to construct the metrics config as a separated field?

[liqinrae]

@jackye1995 yes you are right. There is no need to construct the metrics config since the table properties have already been passed to the writable table handle. I checked the code and we already have functions in place to retrieve metrics related properties so we can make this even simpler. Will add a fix to this, thanks!

[jackye1995]

error message does not match.

Btw, I think it makes more sense to pass in the MetricsConfig to the constructor instead of storageProperties, because file format is also derived from storageProperties and we are passing it in separately.

[liqinrae]

@jackye1995 Sorry you meant passing in the MetricsConfig to the pagesink or IcebergWritableTableHandle constructor?

The file format is derived from the property map and then passed to the writable table handle as a separate component, which is how we passed inMetricsConfig initially.

[jackye1995]

to the page sink. But I don't have a strong opinion on this, there are many duplicated information passed across the codebase as of today. We can just fix the error message first.

[liqinrae]

@liqinrae can you please check the Ci build results?

@findepi Yes seems like there is new changes from other PRs. Will merge them and fix the CI build.

[findepi]

@liqinrae can you please check the Ci build results?

[jackye1995]

looks good to me

[findepi]

Please squash your commits.

When doing so, try not to change the base commit:

git rebase -i $(git merge-base head master)

[findepi]

MetricsMode is an interface, open for extensions. The fact that something isn't Counts doesn't mean it's Full.

Also, there is no javadoc indicating .get() returns same instance every time, so using .equals() would be more appropriate here.

this should be handled in a defensive manner:

if (metricsMode.equals(MetricsModes.None.get())) {
 // nothing to do
}
else if (metricsMode.equals(MetricsModes.Counts.get()) {
 .. collect counts 
}
else if (metricsMode.equals( full )) {
  ...
}
else {
    throw new UnsupportedOperationException("Unsupported metrics mode: " + metricsMode);
}

[liqinrae]

@findepi The conditional statement here handles the case where the metricsMode is truncate(length) or full. Both of these two cases need to store min/max except that truncate need to apply truncation if needed.

All of the metrics modes other than None need to collect counts. There would be duplication of code if we are to handle each mode separately. Would adding a verify function here more feasible if we want to handle it more defensively?

[findepi]

binary (varbinary) and fixed too

https://github.com/apache/iceberg/blob/2e4684740b56f6d80328f50b7e7fef0863c262e7/parquet/src/main/java/org/apache/iceberg/parquet/ParquetUtil.java#L269

(we don't support creating tables with fixed, but i think we do support writes to such tables)

[liqinrae]

@findepi I was referring to the OrcMetrics implementation here: https://github.com/apache/iceberg/blob/ddc5aff9167c7e367e2b4313b91e55451f09ef16/orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java#L281
Seems like it only added truncation for string bound in the orc format.

[findepi]

because they don't produce any stats for binary for ORC, see

trino/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcConnectorTest.java

Line 31 in 88ef2ca

return !(typeName.equalsIgnoreCase("varbinary"));

right?

[findepi]

still, not having an if for varbinary doesn't seem future proof, does it?

[liqinrae]

@findepi yes I agree. I'll add varbinary here.

[findepi]

if MetricsMode is != Full (eg Counts, None), no min/max should be stored.

again, this should be handled in defensive manner, instead of if-ing on a Full class

[findepi]

lowerBounds.get(9) is the actual, while SELECT substring(min(comment), 1, 16) FROM tpch.tiny.orders is the expected. The expected/actual are reversed here.

[liqinrae]

@findepi I tried reversing the order for all the assertQuery in the test function but encountered failures when using SELECT substring(min(comment), 1, 16) FROM tpch.tiny.orders as the expected sql statement. Checked other test files and looks like they placed all select statement as the expected and result as the actual.

[findepi]

we shouldn't use assertQuery at all, when we want to assert a value that we have at hand.

this is preexisting, so ok to followup

[liqinrae]

Hi @findepi @jackye1995 I addressed the comments and left a few replies on this PR. Please let me know your thoughts. Thanks!

[findepi]

we shouldn't use assertQuery at all, when we want to assert a value that we have at hand.

this is preexisting, so ok to followup

[findepi]

assert on upperBounds.get(9) without taking substring

on the left side, use substring(max(comment), 1, 15) || '...' to construct the expected value

[findepi]

@liqinrae have you had time to update this for my comments?

[liqinrae]

@liqinrae have you had time to update this for my comments?

@findepi Hey thanks for reviewing my changes. Updated the PR for your comment and also replaced assertQuery in the preexisting test class with the actual min/max value. Please review and let me know your thoughts. Thanks.