Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/11048#issuecomment-191533739
@viirya Thanks for working on this. The overall approach is very reasonable
but a few things can be improved:
- many files need to be moved to the new packages
- many unused imports in all the files
- many fields and methods can be marked as private, but are not
- the splitting of commands into 2 files is kind of arbitrary. IIUC all
native commands we currently pass to Hive directly today are grouped under
`ddl.scala`, and the rest are grouped under `commands.scala`. In the future,
however, we would like to handle all the DDLs ourselves, so this division won't
make sense anymore.
- general lack of comments in this area of the code makes things difficult
to follow (not your fault)
Additionally I think the reason why this patch is so big is because we
moved a lot of files. The moving itself can be done in a separate PR, which
would reduce the diff significantly and make the patch easier to review.
Addressing all the outstanding comments will likely take a long time. Would
you mind that I take this over? I'll be sure to include you as the original
author in the final commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
