-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Ignore disappeared datasets during listing tables in BigQuery #9954
Ignore disappeared datasets during listing tables in BigQuery #9954
Conversation
cd86096
to
3994e64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits
} | ||
catch (BigQueryException e) { | ||
if (e.getCode() == 404 && e.getMessage().contains("Not found: Dataset")) { | ||
log.debug("Dataset disappeared during listing operation: %s", remoteSchemaName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment like in
trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
Lines 600 to 603 in 1a7d82e
catch (TableNotFoundException | AccessDeniedException e) { | |
// table disappeared during listing operation or user is not allowed to access it | |
// these exceptions are ignored because listTableColumns is used for metadata queries (SELECT FROM information_schema) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW this seems to be a pre-existing case in JDBC metadata too. There we ignore only missing tables, not schemas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I checked JDBC module before sending this PR, it doesn't have the same issue. Can you point out the exact issue place? The DefaultJdbcMetadata
snippet condition is little misleading (donwstream method won't throw TableNotFoundException), but the entire listing logic looks correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It throws JDBC_ERROR wrapping the original exception that was encountered instead of ignoring failures during listing.
See BaseJdbcClient#getTableNames -
trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
Lines 226 to 228 in 338425d
catch (SQLException e) { | |
throw new TrinoException(JDBC_ERROR, e); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getTables()
in the try block calls DatabaseMetaData.getTables()
method, so it won't throw an exception even if the schema doesn't exist in my understanding. IdentifierMapping
might lead to the failure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if it was unclear. I meant that the existing JDBC code can fail if DatabaseMetaData#getTAbles
fails which we know to be the case with MemSQL for example. It's purely theoretical but it can happen IIUC?
// filter ambiguous tables | ||
boolean isAmbiguous = bigQueryClient.toRemoteTable(projectId, remoteSchemaName, table.getTableId().getTable().toLowerCase(ENGLISH), tables) | ||
.filter(RemoteDatabaseObject::isAmbiguous) | ||
.isPresent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use ifPresentOrElse
here to avoid assignment and the if...else
? Would it look better?
a887d19
to
bd3ae8d
Compare
bd3ae8d
to
2768b0d
Compare
No description provided.