ngsg commented on PR #6013:
URL: https://github.com/apache/hive/pull/6013#issuecomment-3231802468
Thanks a lot for the hard work on this PR. I haven't fully reviewed all the
changes yet (the diff is quite large), but let me leave a few comments for now:
- In some cases, handling default partition name isn't necessary. I would
suggest introducing new methods with Preconditions for those cases to avoid
passing {tableParam, conf} or a default partition name. For example, when
calling `Warehouse#escapePathName` in `Warehouse#makePartNameUtil`:
`e.getKey()` doesn't require a default partition name, and `e.getValue()` is
guaranteed not to be empty or null.
- Extending the above point, I would suggest passing just a default
partition name instead of both tableParam and configuration whenever possible.
For example, since `FileUtils#escapePathNames` already takes `defaultPath`, it
would be better to require callers to always pass a proper default partition
name. This way, we propagate only the minimal required information and lower
the risk of human error when dealing with a potentially mutable map instance.
- Regarding `InsertHiveLocksCommand`, I would suggest considering
alternatives. Do we really need to ship the entire content of tableParams over
`LockComponent`? Could we instead just send the default partition name, or
ensure that the partition name is not null or empty? (We might even assume
there is no empty string or null based on the implementation of
`com.google.common.base.Splitter$MapSplitter#split`, though this code is
outside our control.)
- I haven't fully thought this through, but since the changes now affect the
Thrift interface, I would like to hear others' opinions. What do you think
about extending the definition of `Table` and `Partition` in the Thrift
interface instead of using tableParams? In this way, we might be able to
simplify default partition determining steps.
Also, regarding the error in Javadoc, it seems the issue comes from the
javadoc comment in `LockComponentBuilder#setTableParams`: the parameter name
should be `tableParams`, not `tableName`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
