Skip to content

Do not hide errors, which trigger manual delta check point file read #28808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ebyhr merged 1 commit into from
Mar 25, 2026
+16 −0
Merged

Do not hide errors, which trigger manual delta check point file read #28808

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...lta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/TransactionLogParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import dev.failsafe.Failsafe;
import dev.failsafe.RetryPolicy;
import io.airlift.json.JsonMapperProvider;
Expand All @@ -33,10 +34,12 @@
import io.trino.spi.type.Type;
import jakarta.annotation.Nullable;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.math.BigDecimal;
import java.nio.file.NoSuchFileException;
import java.time.Duration;
import java.time.LocalDate;
import java.time.LocalDateTime;
Expand Down Expand Up @@ -287,10 +290,23 @@ private static Optional<LastCheckpoint> tryReadLastCheckpoint(TrinoFileSystem fi
// _last_checkpoint file was not found, we need to find latest checkpoint manually
// ideally, we'd detect the condition by catching FileNotFoundException, but some file system implementations
// will throw different exceptions if the checkpoint is not found
if (isFileNotFoundException(e)) {
return Optional.empty();
}
// it could be situation, like access deny or other permission failure error, which actually should not trigger manual check point read
// because we can not be sure, which exceptions could appear here, at least we should not silently hide them
// TODO after we collect more of such situations we could add exclusion rules here
log.warn(e, "Failed to read Delta Lake last checkpoint file %s, falling back to manual checkpoint discovery", checkpointPath);
Copy link
Copy Markdown
Member

@ebyhr ebyhr Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why warning log? What is the expected action when users see this message?

Copy link
Copy Markdown
Contributor Author

@vlad-lyutenko vlad-lyutenko Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @ebyhr, because of this comment :

// but some file system implementations
// will throw different exceptions if the checkpoint is not found

we can not be 100% sure that this will bring us to the point, where reading check point file manually will fail.

But generally should.

Imagine situation (before this PR), some changes on directory access permission

  • we got exception here - strict permission error
  • silently hide it (because we think that it's legal and we just need to find latest check point manually) and return Optional.empty() here
  • we try to find it and read
  • and got completely another exception smth like Metadata not found in transaction log for, because we hide original permission error.

So with this change user will be able to understand what is original cause.
Maybe log level should be error, but in this case some legit exceptional situation will be highlighted as error.

return Optional.empty();
}
}

private static boolean isFileNotFoundException(Throwable throwable)
{
return Throwables.getCausalChain(throwable).stream()
.anyMatch(cause -> cause instanceof FileNotFoundException || cause instanceof NoSuchFileException);
}

public static long getMandatoryCurrentVersion(TrinoFileSystem fileSystem, String tableLocation, long readVersion)
throws IOException
{
Expand Down