Invalidate ORC metadata cache based on file modification time

[nmahadevuni]

Description

Supports invalidation of ORC metadata cache entries based on the file modification time.

Motivation and Context

For traditional Hive ORC tables, after Presto reads and caches ORC metadata, if other tools modify the ORC files in-place. Presto would try to read old offsets in the new file resulting in errors like "Malformed ORC file..". This PR implements support to invalidate the cache entries based on the file modified timestamp.

Impact

No impact.

Test Plan

Added tests to AbstractTestOrcReader.java.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add support for ORC metadata cache invalidation based on file modification time. :pr:`24346`

[ZacBlanco]

One concern I have with this solution is that the file modification times are also cached by the directory lister implementation. If the caching directory lister is enabled, then we won't get updated file modification times, and this solution won't solve the original issue.

I traced the call paths to where we populate the fileLastModified time and it comes from the result of directoryLister.list inside of StoragePartitionLoader.java. If the listing result is cached, we won't get the updated file modification time. "Fixing" the issue would mean bypassing the listing cache which would also have a pretty significant impact on performance. It might be easier to just provide a procedure to invalidate the cache - either through a JMX procedure or SQL procedure.

[steveburnett]

Please add the PR number to the release note entry, and nit rephrasing following the Order of changes in the Release Notes Guidelines.

== RELEASE NOTES ==

General Changes
* Add support for ORC metadata cache invalidation based on file modification time. :pr:`24346`

[nmahadevuni]

One concern I have with this solution is that the file modification times are also cached by the directory lister implementation. If the caching directory lister is enabled, then we won't get updated file modification times, and this solution won't solve the original issue.

I traced the call paths to where we populate the fileLastModified time and it comes from the result of directoryLister.list inside of StoragePartitionLoader.java. If the listing result is cached, we won't get the updated file modification time. "Fixing" the issue would mean bypassing the listing cache which would also have a pretty significant impact on performance. It might be easier to just provide a procedure to invalidate the cache - either through a JMX procedure or SQL procedure.

Agree. All caches which do time based invalidation will come with the caveat that file list caching may interfere with its functioning. There is a stored procedure invalidate_directory_list_cache() which users can use to flush the file list cache.

[nmahadevuni]

@ZacBlanco Addressed the comments. Please review. @agrawalreetika @pratyakshsharma can you please have a look?

[agrawalreetika]

Thanks for making the changes. LGTM

[nmahadevuni]

@hantangwangd Can you please review this?

[hantangwangd]

Thanks for the change, overall looks good to me, this can alleviate a significant portion of the problems caused by cache stale.

Bring one question for discussing: when we enable the cached directoryLister and a new file is added into any partition of a table through an external application without modify existing ones, do we still unable to recognize it?

[hantangwangd]

A little question, what is more appropriate when cacheableSlice.getFileModificationTime() > fileModificationTime? Is it intentional to invalidate and refill the cache in such scenario?

[nmahadevuni]

We are assuming that new fileModificationTime will always be same or more recent than cacheableSlice.getFileModificationTime().

[hantangwangd]

That's ok.

[nmahadevuni]

Thanks for the change, overall looks good to me, this can alleviate a significant portion of the problems caused by cache stale.

Bring one question for discussing: when we enable the cached directoryLister and a new file is added into any partition of a table through an external application without modify existing ones, do we still unable to recognize it?

Right, it will not recognize the new file because the CachingDirectoryLister will have the stale file list.

[hantangwangd]

Right, it will not recognize the new file because the CachingDirectoryLister will have the stale file list.

Thanks, glad to confirm this.

[nmahadevuni]

@hantangwangd @ZacBlanco Can one of you merge this please? I see the merge button is disabled for me even after approvals.

[ZacBlanco]

We need a committer approval - @tdcmeehan could you help?

[shangm2]

Hey guys. We are seeing the following errors due to the conflict between this pr and #24351. @nmahadevuni @tdcmeehan

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-cli) on project presto-orc: Compilation failure /Users/shangma/workspace/presto/presto-orc/src/main/java/com/facebook/presto/orc/StripeReader.java:[260,75] method getRowIndexes in interface com.facebook.presto.orc.StripeMetadataSource cannot be applied to given types; required: com.facebook.presto.orc.metadata.MetadataReader,com.facebook.presto.orc.metadata.PostScript.HiveWriterVersion,com.facebook.presto.orc.StripeReader.StripeId,com.facebook.presto.orc.StreamId,com.facebook.presto.orc.stream.OrcInputStream,java.util.List<com.facebook.presto.orc.metadata.statistics.HiveBloomFilter>,com.facebook.presto.common.RuntimeStats,long found: com.facebook.presto.orc.metadata.MetadataReader,com.facebook.presto.orc.metadata.PostScript.HiveWriterVersion,com.facebook.presto.orc.StripeReader.StripeId,com.facebook.presto.orc.StreamId,com.facebook.presto.orc.stream.OrcInputStream,<nulltype>,com.facebook.presto.common.RuntimeStats reason: actual and formal argument lists differ in length

[tdcmeehan]

The trivial fix is in #24421