HIVE-29345: Support Alter table command for write ordering. #6256
+510
−43
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SYNTAX: ALTER TABLE table_name SET WRITE ORDERED BY column_name sort_direction NULLS FIRST/LAST, ... EXAMPLE: ALTER TABLE table_order SET WRITE ORDERED BY id desc nulls first, name asc nulls last;
kokila-19
force-pushed
the
alter_natural_order
branch
from
22a148b to
9d5ffb3
Compare
|
deniskuzZ
reviewed
Jan 5, 2026
| * @return List of SortFieldDesc, or null if parsing fails or JSON is empty | ||
| */ | ||
| protected List<SortFieldDesc> parseSortFieldsJSON(String sortOrderJSONString) { | ||
| if (Strings.isNullOrEmpty(sortOrderJSONString) || isZOrderJSON(sortOrderJSONString)) { |
Member
There was a problem hiding this comment.
why do we check isZOrderJSON here if it's always false?
https://github.com/apache/hive/pull/6256/files#diff-427b2ffcf14c9599990dd80ea356f9fabbca2fae484f47ee8a3637263b2fa79eR250
deniskuzZ
reviewed
Jan 5, 2026
|
|
||
| try { | ||
| SortFields sortFields = JSON_OBJECT_MAPPER.reader().readValue(sortOrderJSONString, SortFields.class); | ||
| if (sortFields != null && !sortFields.getSortFields().isEmpty()) { |
Member
There was a problem hiding this comment.
why do we need .isEmpty() check?
deniskuzZ
reviewed
Jan 5, 2026
|
|
||
| public class HiveIcebergMetaHook extends BaseHiveIcebergMetaHook { | ||
| private static final Logger LOG = LoggerFactory.getLogger(HiveIcebergMetaHook.class); | ||
| private static final ObjectMapper JSON_OBJECT_MAPPER = new ObjectMapper(); |
Member
There was a problem hiding this comment.
why do we need this? BaseHiveIcebergMetaHook already defines it
deniskuzZ
reviewed
Jan 5, 2026
| * SortOrder JSON and keep it in DEFAULT_SORT_ORDER for Iceberg to use it. | ||
| */ | ||
| private void setSortOrder(org.apache.hadoop.hive.metastore.api.Table hmsTable, Schema schema, | ||
| protected void setSortOrder(org.apache.hadoop.hive.metastore.api.Table hmsTable, Schema schema, |
deniskuzZ
reviewed
Jan 5, 2026
|
|
||
| // Check if we are setting regular sort order as it needs conversion from Hive JSON to Iceberg SortOrder | ||
| if (propertiesToSet.contains(TableProperties.DEFAULT_SORT_ORDER)) { | ||
| // If the HMS table has Hive SortFields JSON in default-sort-order |
Member
There was a problem hiding this comment.
maybe extract this logic into a separate method. also consider Set due to use of contains
deniskuzZ
reviewed
Jan 5, 2026
| /** | ||
| * Converts Hive SortDirection to Iceberg SortDirection. | ||
| */ | ||
| protected static org.apache.iceberg.SortDirection convertSortDirection(SortFieldDesc.SortDirection direction) { |
deniskuzZ
reviewed
Jan 5, 2026
| } | ||
|
|
||
| // Set other properties excluding default-sort-order which is already processed) | ||
| propertiesToSet.stream() |
Member
There was a problem hiding this comment.
why add filter here and not handle DEFAULT_SORT_ORDER during propertiesToSet iteration?
Map<String, Consumer<String>> handlers = new HashMap<>();
handlers.put(
TableProperties.DEFAULT_SORT_ORDER,
k -> handleDefaultSortOrder(hmsTable, hmsTableParameters)
);
splitter.splitToList(contextProperties.get(SET_PROPERTIES)).forEach(key ->
handlers.getOrDefault(
key,
k -> update.set(k, hmsTableParameters.get(k))
).accept(key)
);
deniskuzZ
reviewed
Jan 5, 2026
| // Regular ORDERED BY - to be implemented in future commit | ||
| throw new SemanticException("Regular ORDERED BY is not yet supported. Only ZORDER is supported."); | ||
| // Handle regular ORDERED BY | ||
| handleRegularOrder(tableName, partitionSpec, orderNode); |
Member
There was a problem hiding this comment.
deniskuzZ
reviewed
Jan 5, 2026
| SortFields sortFields = new SortFields(sortFieldDescList); | ||
| String sortOrderJson; | ||
| try { | ||
| sortOrderJson = JSON_OBJECT_MAPPER.writeValueAsString(sortFields); |
Member
There was a problem hiding this comment.
don't we already have code that handles natural order during create ? can we reuse?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Implemented Natural ordering(write ordered by) ALTER TABLE DDL support for Iceberg tables.
Why are the changes needed?
Existing tables can be converted to natural ordered tables using ALTER command. Note that only the data inserted after alter query are ordered, not existing data.
Does this PR introduce any user-facing change?
yes, supported new syntax
SYNTAX:
ALTER TABLE table_name SET WRITE ORDERED BY column_name sort_direction NULLS FIRST/LAST, ...
EXAMPLE:
ALTER TABLE table_order SET WRITE ORDERED BY id desc nulls first, name asc nulls last;
How was this patch tested?
qtest