Move ViewDefinition to presto-analyzer #18680
Conversation
jainxrohit
force-pushed
the
rj_prux
branch
5 times, most recently
from
a91898c to
d86e11b
Compare
|
That looks like an odd package to move this class to |
There was a problem hiding this comment.
could just move the ' into the next string
There was a problem hiding this comment.
Not sure if I follow.
There was a problem hiding this comment.
Don't concatenate a character literal here. Instead the next string can be "', catalog="
presto-main/src/main/java/com/facebook/presto/sql/analyzer/BuiltInMetadataResolver.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
These methods could use Javadoc
There was a problem hiding this comment.
I will add java doc after one round of review.
There was a problem hiding this comment.
Perhaps explain what the metadata is instead of simply repeating "Metadata resolver provides metadata". E.g. a metadata resolver provides views and table and column names by querying something"
There was a problem hiding this comment.
So a table that doesn't exist returns an empty list here? same a a table with no columns that does exist?
There was a problem hiding this comment.
If the table does not exist, it would throw exception.
There was a problem hiding this comment.
That arguably should be a checked exception. Whether it is or not, it definitely needs explicit Javadoc on this point: what exception is thrown under what circumstances
There was a problem hiding this comment.
needs Javadoc indicating at least where the metadata is coming from
jainxrohit
force-pushed
the
rj_prux
branch
3 times, most recently
from
0609188 to
66234ae
Compare
highker
left a comment
highker
left a comment
There was a problem hiding this comment.
Still reviewing; the first two commits LGTM % one package question
presto-common/src/main/java/com/facebook/presto/common/ViewDefinition.java
Outdated
Show resolved
Hide resolved
presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/MetadataResolver.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
That arguably should be a checked exception. Whether it is or not, it definitely needs explicit Javadoc on this point: what exception is thrown under what circumstances
There was a problem hiding this comment.
The first two argument are unclear to me. Consider renaming.
listBuiltInFunctionsOnly might better be no argument at all. Instead have two methods: listBuiltinFunctions and listAllFunctions
There was a problem hiding this comment.
Don't concatenate a character literal here. Instead the next string can be "', catalog="
There was a problem hiding this comment.
If there's a reason why some return values are optional and others aren't, I'm missing it.
jainxrohit
force-pushed
the
rj_prux
branch
2 times, most recently
from
43a9b87 to
989c5a4
Compare
jainxrohit
changed the title
Moving ViewDefinition to presto-analyzer. ViewDefinition relies on a few classes from Guava, which is an issue as this move was leading to presto-spi depending on guava. Hence guava related classes have been removed from ViewDefinition.
Renaming ConnectorMaterializedViewDefinition to MaterializedViewDefinition.
|
@jainxrohit has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
5 participants
Moving ViewDefinition to presto-analyzer, and renamed ConnectorMaterializedViewDefinition to MaterializedViewDefinition in this change.