Apply Baseline plugin to iceberg-hive #233
Conversation
| } | ||
| } | ||
|
|
||
| protected abstract void close(C client); |
There was a problem hiding this comment.
This one might be questionable. Our code style requires overloaded methods to be together.
There was a problem hiding this comment.
Can we move the other close to just below the abstract methods instead?
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * Copyright 2004 Clinton Begin |
There was a problem hiding this comment.
I am not sure about the header here.
There was a problem hiding this comment.
We need to keep the old header. Apache projects do not change copyright headers, except to add additional notes about modifications.
| <check level="error" class="org.scalastyle.scalariform.NoWhitespaceBeforeLeftBracketChecker" enabled="true"/> | ||
| <check level="error" class="org.scalastyle.scalariform.NoWhitespaceAfterLeftBracketChecker" enabled="true"/> | ||
| <check level="error" class="org.scalastyle.scalariform.ReturnChecker" enabled="true"/> | ||
| <check level="error" class="org.scalastyle.scalariform.ReturnChecker" enabled="false"/> |
There was a problem hiding this comment.
This is a leftover after the PR for Spark.
|
|
||
| if (tableType == null || !tableType.equalsIgnoreCase(ICEBERG_TABLE_TYPE_VALUE)) { | ||
| throw new IllegalArgumentException(format("Invalid tableName, not Iceberg: %s.%s", database, table)); | ||
| throw new IllegalArgumentException(String.format("Invalid tableName, not Iceberg: %s.%s", database, table)); |
There was a problem hiding this comment.
Would it make sense to simply use Preconditions.checkArgument here [and other places where we use IllegalArgumentException] which supports formatting out of the box?
| } catch (NoSuchObjectException e) { | ||
| if (currentMetadataLocation() != null) { | ||
| throw new NoSuchTableException(format("No such table: %s.%s", database, tableName)); | ||
| throw new NoSuchTableException(String.format("No such table: %s.%s", database, tableName)); |
There was a problem hiding this comment.
NoSuchTableException supports string formatting
| } catch (TException e) { | ||
| throw new RuntimeException(format("Failed to get table info from metastore %s.%s", database, tableName), e); | ||
| String errMsg = String.format("Failed to get table info from metastore %s.%s", database, tableName); | ||
| throw new RuntimeException(errMsg, e); |
There was a problem hiding this comment.
We have a RuntimeMetaException which supports formatting, should we create a RuntimeTException to supporting formatting ?
There was a problem hiding this comment.
Good ideas. I'm fine either way with this. It would be nice to clean this up now, but I am fine with doing it in a separate commit since this is supposed to clean up formatting, not make substantive changes.
| if (!Objects.equals(currentMetadataLocation(), metadataLocation)) { | ||
| throw new CommitFailedException(format("metadataLocation = %s is not same as table metadataLocation %s for %s.%s", | ||
| currentMetadataLocation(), metadataLocation, database, tableName)); | ||
| String errMsg = String.format("metadataLocation = %s is not same as table metadataLocation %s for %s.%s", |
There was a problem hiding this comment.
Same, CommitFailedException supports formatting. We do not need to wrap it with String.fromat
| private final List<FieldSchema> columns(Schema schema) { | ||
| return schema.columns().stream().map(col -> new FieldSchema(col.name(), HiveTypeConverter.convert(col.type()), "")).collect(Collectors.toList()); | ||
| private List<FieldSchema> columns(Schema schema) { | ||
| return schema.columns().stream() |
There was a problem hiding this comment.
Can we replace this with Lists.transform , since we prefer using Guava's HOFs
| if (!state.equals(LockState.ACQUIRED)) { | ||
| throw new CommitFailedException(format("Could not acquire the lock on %s.%s, " + | ||
| "lock request ended in state %s", database, tableName, state)); | ||
| throw new CommitFailedException(String.format("Could not acquire the lock on %s.%s, " + |
|
@aokolnychyi, this looks good to me except for the copyright header change. That needs to be reverted. As for the problems that @rdsr noted, I'm okay fixing them either here or in a follow-up to keep this focused on baseline changes. |
aokolnychyi
force-pushed
the
hive-code-style
branch
from
2bef383 to
bf03ea9
Compare
|
@aokolnychyi, thanks for fixing this! I made some of the suggested changes in #240. Let's get #240 and #239 in and then follow up with fixes for the remaining items. |
3 participants
This PR applies Baseline to iceberg-hive and resolves #157.