HIVE-29413: Generalise column related APIs in Table.java#6413
HIVE-29413: Generalise column related APIs in Table.java#6413ramitg254 wants to merge 37 commits into
Conversation
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
|
@ramitg254 please take a look: 9e7535c. I would suggest following similar approach |
|
but here we are creating separate method getEffectivePartCols() and leaving getPartCols() as it is, which as per our discussion on that closed pr we shouldn't do that, and only go ahead with updating getPartCols() |
Where did I say that? The ask was to keep the original method unchanged. same here |
|
oh I got confused due to this comment: #6337 (comment) in which getSupportedPartCols() was just separate method similar to getEffectivePartCols() |
|
I am fine with that earlier approach as well but recently I saw this one: https://issues.apache.org/jira/browse/HIVE-29525 so I thought we should have unified getPartCols() and getCols() which gives similar results as native hive tables as first step towards solving this after that those plan logics can be taken care of later on when that ticket will be addressed. please share your thoughts on this idea |
# Conflicts: # iceberg/iceberg-handler/src/test/results/positive/llap/iceberg_bucket_map_join_7.q.out
Change-Id: I09ffba356ac47e3416c8b6717e8671d2cb6432b8
Change-Id: I6812d068be702edf01b158c3e7eacadeadbd5c2b
Change-Id: I6f5d81c5be00e080753efd5e8157b14fcfde2dde
Change-Id: I5a95791b4d756fe949ee4697204be9de3025c6b5
Change-Id: I852cf156a1a5ac20525bb9376f4cbb5783b8e1e3
Change-Id: Ib1542e38c65c7811074b97d7da7aaf780f3895dd
Change-Id: Ie3440239248870fb44c6e956d5be10271b28a1a0
Change-Id: Ic8f6d6c391ba49e12de7d04b7bb20542d879b0a3
Change-Id: I2017f7c771f8f611b5d8900d3e0276817842b41f
| List<FieldSchema> partCols = hmsTable.getPartCols(); | ||
| return partCols != null ? partCols : Collections.emptyList(); | ||
| if (BaseHiveIcebergMetaHook.isIcebergView(hmsTable.getTTable()) || | ||
| !MetaStoreUtils.isIcebergTable(hmsTable.getParameters())) { |
There was a problem hiding this comment.
why do we check !MetaStoreUtils.isIcebergTable?
There was a problem hiding this comment.
this is the same guard as isTableTypeSet with already available method, please check af21d56
There was a problem hiding this comment.
just strange that semantically same methods are in diff classes (maybe in a follow-up we can refactor):
- BaseHiveIcebergMetaHook.isIcebergView
- MetaStoreUtils.isIcebergTable
There was a problem hiding this comment.
both the methods are concerned with table params so moved them along with combined helper to MetaStoreUtils
Change-Id: I87dc65eb942e3db1b1a0d9f2608f1b88b44b11e6
Change-Id: Ic0fefede8b079205c5580a31f59f195b6b1e94e0
|
3 participants
What changes were proposed in this pull request?
added getEffectivePartCols() in most places possible to avoid code duplication.
Why are the changes needed?
getPartCols() does not have support for iceberg tables.
Does this PR introduce any user-facing change?
No
How was this patch tested?
ci tests and local build