You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org> on 2026/10/07 18:07:54 UTC
[PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Aggarwal-Raghav opened a new pull request, #6856:
URL: https://github.com/apache/hive/pull/6856
### What changes were proposed in this pull request?
Commits made in upsteam iceberg b/w 1.11.0 and 1.12.0 in hive-metastore module:
```
❯ git log apache-iceberg-1.11.0..apache-iceberg-1.12.0 --oneline -- hive-metastore
e74344ca56 Core, Hive: Commit manifest list encryption keys with the snapshot that uses them (#17984)
0630297ef2 Core, Hive: Fix NPE in commit status check when a create commit fails (#17464)
20d616d6a1 Core, Hive, Spark: Auto close JsonGenerator using try-with-resources (#17931)
4a1513f1f2 API, AWS, BigQuery, Core, Flink, Hive, Nessie, ORC, Parquet, Spark: Use Locale.ROOT instead of Locale.ENGLISH for case conversion (#17442)
787076dc0c Hive: Fix integer overflow in HMS createTime and lastAccessTime (#16620)
8f4843394c Hive: Use server-side filter to list Iceberg tables in HiveCatalog (#17317)
0ddbeead67 Core, AWS, GCP, Dell, Hive: Fix FileIO leaks and standardize close() across Catalog implementations (#16862)
687c58fc7c Core: v4 table metadata location should be optional (#16572)
```
**Important points:**
1. FileWriterFactory Refactor: Upstream Iceberg 1.12.0 deleted the BaseFileWriterFactory and GenericAppenderFactory classes. To adapt, HiveFileWriterFactory was refactored https://github.com/apache/iceberg/pull/17696
2. Positional Delete API Updates: Iceberg 1.12.0 removed the .row() getter and .set(path, pos, row) setter from PositionDelete to prepare for V3 Puffin Deletion Vectors. https://github.com/apache/iceberg/pull/17706
3. Updated `HMSCatalogAdapter.java` to parse and pass the new SnapshotMode (ALL, REFS, NONE) into `CatalogHandlers.loadTable`
4. Removed `positionDeleteRowSchema` after refactor
### Why are the changes needed?
Upgrade Iceberg to 1.12.0
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Will see CI outcome
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245366484
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCatalogAdapterSnapshotMode.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.iceberg.BaseTable;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableOperations;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestHMSCatalogAdapterSnapshotMode {
+
+ private static final String TABLE_PATH = "v1/namespaces/accounting/tables/my_table";
+
+ private HMSCatalogAdapter adapter;
+ private HttpServletResponse response;
+ private StringWriter stringWriter;
+ private TableMetadata initialMetadata;
+
+ @BeforeEach
+ void setup() throws Exception {
+ Catalog catalog =
+ Mockito.mock(
+ Catalog.class,
+ Mockito.withSettings().extraInterfaces(SupportsNamespaces.class, ViewCatalog.class));
+
+ Schema schema = new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ Snapshot snap1 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap1.snapshotId()).thenReturn(1L);
+ Mockito.when(snap1.sequenceNumber()).thenReturn(1L);
+ Mockito.when(snap1.timestampMillis()).thenReturn(1000L);
+
+ Snapshot snap2 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap2.snapshotId()).thenReturn(2L);
+ Mockito.when(snap2.sequenceNumber()).thenReturn(2L);
+ Mockito.when(snap2.timestampMillis()).thenReturn(2000L);
+
+ initialMetadata =
+ TableMetadata.buildFromEmpty(2)
+ .setLocation("file:///tmp/table")
+ .addSchema(schema)
+ .setDefaultPartitionSpec(PartitionSpec.unpartitioned())
+ .setDefaultSortOrder(SortOrder.unsorted())
+ .addSnapshot(snap1)
+ .addSnapshot(snap2)
+ .setBranchSnapshot(2L, SnapshotRef.MAIN_BRANCH)
+ .discardChanges()
+ .withMetadataLocation("file:///tmp/table/metadata.json")
+ .build();
+
+ BaseTable table = Mockito.mock(BaseTable.class);
+ TableOperations operations = Mockito.mock(TableOperations.class);
+ Mockito.when(table.operations()).thenReturn(operations);
+ Mockito.when(operations.current()).thenReturn(initialMetadata);
+ Mockito.when(catalog.loadTable(Mockito.any(TableIdentifier.class))).thenReturn(table);
+
+ adapter = new HMSCatalogAdapter("test_catalog", catalog, null, Collections.emptyList());
+
+ response = Mockito.mock(HttpServletResponse.class);
+ stringWriter = new StringWriter();
+ Mockito.when(response.getWriter()).thenReturn(new PrintWriter(stringWriter));
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (adapter != null) {
+ adapter.close();
+ }
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamOmitted() throws Exception {
+ // When snapshots query parameter is omitted (null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res = adapter.execute(HTTPMethod.GET, TABLE_PATH, null, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots including historical ones");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamNull() throws Exception {
+ // When query params map is empty (snapshots is null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, Collections.emptyMap(), null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots when query parameter map has no snapshots key");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testSnapshotModeAll() throws Exception {
+ // Standard query parameter: snapshots=all
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "all");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2, res.tableMetadata().snapshots().size(), "Mode ALL should return all snapshots");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(1L), "Historical snapshot 1 should be present");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be present");
+ }
+
+ @Test
+ void testSnapshotModeRefs() throws Exception {
+ // Standard query parameter: snapshots=refs
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "refs");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 1,
+ res.tableMetadata().snapshots().size(),
+ "Mode REFS should suppress historical snapshots");
+ Assertions.assertNull(
+ res.tableMetadata().snapshot(1L),
+ "Historical snapshot 1 should be suppressed in REFS mode");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be retained in REFS mode");
+ }
+
+ @Test
+ void testNegativeInvalidSnapshotMode() throws Exception {
+ // Unknown mode value: snapshots=invalid
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "invalid");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for an invalid snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ String responseBody = stringWriter.toString();
+ Assertions.assertTrue(
+ responseBody.contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException");
+ Assertions.assertTrue(
+ responseBody.contains("INVALID"), "Error response message should mention the invalid mode");
+ }
+
+ @Test
+ void testNegativeEmptySnapshotMode() throws Exception {
+ // Empty string value: snapshots=""
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for empty snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ Assertions.assertTrue(
+ stringWriter.toString().contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException for empty mode");
+ }
+
+ @Test
+ void testNegativeNumericSnapshotMode() throws Exception {
Review Comment:
please check all these test-cases are actually needed, maybe extract common parts
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6133518643
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6060442559
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222673418
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergAcidUtil.java:
##########
@@ -138,7 +138,8 @@ public static PositionDelete<Record> getPositionDelete(Record rec, Record rowDat
rowData.set(i - dataOffset, rec.get(i));
}
- positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L), rowData);
+ positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L));
+ positionDelete.set(2, rowData);
Review Comment:
they did, please check important points in PR description. I've mentioned breaking changes in this PR's description
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241329803
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = rowDataTemplate;
Review Comment:
As per javadoc of `IcebergAcidUtil.getPositionDelete()`, the `rowDataTemplate` is modified in-place
```
Based on `rec` the method creates a position delete object, and also populates the data filed of `rowData` with the field values from `rec`.
```
So, positionDelete.row() contains same what rowDataTemplate contains . As `positionDelete.row()` is gone therefore, I replaced it with rowDataTemplate
Please let me know If i misunderstood your comment!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222799124
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -352,6 +363,37 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
Review Comment:
old listIcebergTables should be dropped?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4223002942
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -246,28 +246,14 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
boolean newTable = base == null;
encryptionPropsFromMetadata(metadata.properties());
boolean keepHiveStats = conf.getBoolean(ConfigProperties.KEEP_HIVE_STATS, false);
- final TableMetadata tableMetadata;
- EncryptionManager encrManager = encryption();
- if (encrManager instanceof StandardEncryptionManager) {
Review Comment:
moved to `core/src/main/java/org/apache/iceberg/SnapshotProducer.java` , i guess, https://github.com/apache/iceberg/pull/17984
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241244412
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergDeleteWriter.java:
##########
@@ -61,10 +61,10 @@ public void write(Writable row) throws IOException {
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(rec, rowDataTemplate, isMergeTask);
int specId = IcebergAcidUtil.parseSpecId(rec);
PartitionKey partitionKey = isMergeTask ? IcebergAcidUtil.parsePartitionKey(rec) :
- partition(positionDelete.row(), specId);
+ partition(rowDataTemplate, specId);
Review Comment:
same thing
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6124175854
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [1.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245307480
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -66,45 +81,68 @@ public void appendToTable(DataFile... dataFiles) {
append.commit();
}
+ public void appendToTable(DataFile... dataFiles) {
+ appendToTable(null, dataFiles);
+ }
+
public void appendToTable(List<Record> records) throws IOException {
- appendToTable(null, records);
+ appendToTable(null, null, records);
+ }
+
+ public void appendToTable(String branch, List<Record> records) throws IOException {
+ appendToTable(null, branch, records);
+ }
+
+ public void appendToTable(StructLike partition, String branch, List<Record> records)
+ throws IOException {
+ appendToTable(branch, writeFile(partition, records));
}
public void appendToTable(StructLike partition, List<Record> records) throws IOException {
appendToTable(writeFile(partition, records));
}
+ public DataFile writeFile(List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = temp.resolve("generic-appender-test-" + UUID.randomUUID()).toFile();
+ return appendToLocalFile(table, file, fileFormat, null, records, conf);
+ }
+
public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
- File file = tmp.newFile();
- Assert.assertTrue(file.delete());
+ File file =
+ temp.resolve("generic-appender-partition-test-" + UUID.randomUUID()).toFile();
Review Comment:
could you please move to constant "generic-appender-partition-test-"
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222981280
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -19,23 +19,33 @@
package org.apache.iceberg.mr.hive.writer;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.util.Map;
import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.MetricsConfig;
+import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.StructLike;
import org.apache.iceberg.Table;
-import org.apache.iceberg.avro.Avro;
-import org.apache.iceberg.data.BaseFileWriterFactory;
+import org.apache.iceberg.data.GenericFileWriterFactory;
import org.apache.iceberg.data.Record;
-import org.apache.iceberg.data.avro.DataWriter;
-import org.apache.iceberg.data.orc.GenericOrcWriter;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
-import org.apache.iceberg.orc.ORC;
+import org.apache.iceberg.deletes.EqualityDeleteWriter;
+import org.apache.iceberg.deletes.PositionDeleteWriter;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.VariantUtil;
-class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
+class HiveFileWriterFactory implements FileWriterFactory<Record> {
Review Comment:
no worries
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4223277652
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -352,6 +363,37 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
Review Comment:
in Upstream iceberg `listIcebergTables` is present. `listViews` is making use of `listIcebergTables`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6111476555
@Aggarwal-Raghav please review sonar issues, seem related to the current changes
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6111657313
> @Aggarwal-Raghav please review sonar issues, seem related to the current changes
@deniskuzZ , Need 1 suggestion from you on SonaType,
1. HiveCatalog.java : Its in sync (code wise not formatting wise 😅) with upstream iceberg — NIT , let me know if we want to make this small change.
2. TestHiveIcebergCRUD.java – testReadAndWriteFormatV2Partitioned_PosDelete_**RowSupplied**, testReadAndWriteFormatV2Partitioned_PosDelete_**RowNotSupplied** are using row() and as test name suggest it is testing that functionality. I'll remove these test ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245317456
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
Ok, so, FileHelpers and GenericAppenderHelper are sync fully from upstream https://github.com/apache/hive/pull/6856#issuecomment-6111657313
Attaching URL for your reference:
1. GenericAppenderHelper: https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
2. https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
I'll try to removed them if possible and replace them with iceberg-data test scope dependecy in a follow-up PR
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222628188
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -139,40 +140,33 @@ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
public static DataFile writeDataFile(Table table, OutputFile out, List<Record> rows) throws IOException {
FileFormat format = defaultFormat(table.properties());
- GenericAppenderFactory factory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory factory = new GenericFileWriterFactory.Builder(table).dataFileFormat(format).build();
- FileAppender<Record> writer = factory.newAppender(out, format);
- try (Closeable toClose = writer) {
- writer.addAll(rows);
+ DataWriter<Record> writer = factory.newDataWriter(encrypt(out), table.spec(), null);
+ try (writer) {
+ for (Record row : rows) {
Review Comment:
same
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222692927
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
+ .dataFileFormat(dataFileFormat)
+ .dataSchema(dataSchema)
+ .dataSortOrder(dataSortOrder)
+ .deleteFileFormat(deleteFileFormat)
+ .equalityFieldIds(equalityFieldIds)
+ .equalityDeleteRowSchema(equalityDeleteRowSchema)
+ .equalityDeleteSortOrder(equalityDeleteSortOrder);
+
+ this.delegate = builder.build();
}
static Builder builderFor(Table table) {
return new Builder(table);
}
@Override
- protected void configureDataWrite(Avro.DataWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureEqualityDelete(Avro.DeleteWriteBuilder builder) {
-
- }
-
- @Override
- protected void configurePositionDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureDataWrite(Parquet.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
- // Configure variant shredding if enabled and a sample record is available
- if (VariantUtil.shouldUseVariantShredding(properties, dataSchema())) {
- builder.variantShreddingFunc(VariantUtil.variantShreddingFunc(sampleRecord, dataSchema()));
+ public DataWriter<Record> newDataWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition) {
+ if (dataFileFormat == FileFormat.PARQUET && VariantUtil.shouldUseVariantShredding(properties, dataSchema)) {
Review Comment:
could we move (dataFileFormat == FileFormat.PARQUET) inside VariantUtil.shouldUseVariantShredding ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222666301
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergDeleteWriter.java:
##########
@@ -61,10 +61,11 @@ public void write(Writable row) throws IOException {
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(rec, rowDataTemplate, isMergeTask);
int specId = IcebergAcidUtil.parseSpecId(rec);
PartitionKey partitionKey = isMergeTask ? IcebergAcidUtil.parsePartitionKey(rec) :
- partition(positionDelete.row(), specId);
+ partition(positionDelete.get(2, Record.class), specId);
if (skipRowData) {
// Set null as the row data as we intend to avoid writing the actual row data in the delete file.
- positionDelete.set(positionDelete.path(), positionDelete.pos(), null);
+ positionDelete.set(positionDelete.path(), positionDelete.pos());
+ positionDelete.set(2, null);
Review Comment:
same :) what is this 2?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241236856
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = rowDataTemplate;
Review Comment:
i don't think it's ok. partition(rowData, specId)is determined based on actual data
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241229889
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = rowDataTemplate;
Review Comment:
does `rowDataTemplate` come from iceberg base writer?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222658828
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/test/utils/HiveIcebergTestUtils.java:
##########
@@ -407,12 +410,14 @@ public static DeleteFile createPositionalDeleteFile(Table table, String deleteFi
partitionKey.partition(record);
}
- PositionDeleteWriter<Record> posWriter = appenderFactory.newPosDeleteWriter(outputFile, fileFormat, partitionKey);
- try (PositionDeleteWriter<Record> writer = posWriter) {
+ PositionDeleteWriter<Record> posWriter = writerFactory
+ .newPositionDeleteWriter(outputFile, table.spec(), partitionKey);
+ try (posWriter) {
deletes.forEach(del -> {
PositionDelete positionDelete = PositionDelete.create();
- positionDelete.set(del.path(), del.pos(), del.row());
- writer.write(positionDelete);
+ positionDelete.set(del.path(), del.pos());
+ positionDelete.set(2, del.get(2, Record.class));
Review Comment:
The `.set(path, pos, row)` was removed in https://github.com/apache/iceberg/pull/17696 which was used by V2 spec, but to similuate the same behaviour as users will have V2 tables, the workaround/new way is picked from iceberg UT's
please check this: https://github.com/apache/iceberg/blob/213a4b557db8c0a9b1eea90b908a4e6f3b1cbdf5/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java#L582-L589
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241257208
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java:
##########
@@ -378,7 +379,12 @@ private RESTResponse tableExists(Map<String, String> vars) {
private LoadTableResponse loadTable(Map<String, String> vars) {
TableIdentifier ident = identFromPathVars(vars);
- return CatalogHandlers.loadTable(catalog, ident);
+ String snapshots = vars.get(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER);
+ RESTCatalogProperties.SnapshotMode mode = RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT;
+ if (snapshots != null) {
+ mode = RESTCatalogProperties.SnapshotMode.valueOf(snapshots.toUpperCase(Locale.ROOT));
Review Comment:
No, will add.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6113759691
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6111859440
> > @Aggarwal-Raghav please review sonar issues, seem related to the current changes
>
> @deniskuzZ , Need 1 suggestion from you on SonaType,
>
> 1. HiveCatalog.java : Its in sync (code wise not formatting wise 😅) with upstream iceberg — NIT , let me know if we want to make this small change.
i would keep the iceberg version in that case, easier later to sync
> 2. TestHiveIcebergCRUD.java – testReadAndWriteFormatV2Partitioned_PosDelete_**RowSupplied**, testReadAndWriteFormatV2Partitioned_PosDelete_**RowNotSupplied** are using row() and as test name suggest it is testing that functionality. I'll remove these test ?
could you please test if we would be able to consume legacy v2 with row data. do not need tests for write PosDelete_**RowSupplied**
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245363476
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCatalogAdapterSnapshotMode.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.iceberg.BaseTable;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableOperations;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestHMSCatalogAdapterSnapshotMode {
+
+ private static final String TABLE_PATH = "v1/namespaces/accounting/tables/my_table";
+
+ private HMSCatalogAdapter adapter;
+ private HttpServletResponse response;
+ private StringWriter stringWriter;
+ private TableMetadata initialMetadata;
+
+ @BeforeEach
+ void setup() throws Exception {
+ Catalog catalog =
+ Mockito.mock(
+ Catalog.class,
+ Mockito.withSettings().extraInterfaces(SupportsNamespaces.class, ViewCatalog.class));
+
+ Schema schema = new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ Snapshot snap1 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap1.snapshotId()).thenReturn(1L);
+ Mockito.when(snap1.sequenceNumber()).thenReturn(1L);
+ Mockito.when(snap1.timestampMillis()).thenReturn(1000L);
+
+ Snapshot snap2 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap2.snapshotId()).thenReturn(2L);
+ Mockito.when(snap2.sequenceNumber()).thenReturn(2L);
+ Mockito.when(snap2.timestampMillis()).thenReturn(2000L);
+
+ initialMetadata =
+ TableMetadata.buildFromEmpty(2)
+ .setLocation("file:///tmp/table")
+ .addSchema(schema)
+ .setDefaultPartitionSpec(PartitionSpec.unpartitioned())
+ .setDefaultSortOrder(SortOrder.unsorted())
+ .addSnapshot(snap1)
+ .addSnapshot(snap2)
+ .setBranchSnapshot(2L, SnapshotRef.MAIN_BRANCH)
+ .discardChanges()
+ .withMetadataLocation("file:///tmp/table/metadata.json")
+ .build();
+
+ BaseTable table = Mockito.mock(BaseTable.class);
+ TableOperations operations = Mockito.mock(TableOperations.class);
+ Mockito.when(table.operations()).thenReturn(operations);
+ Mockito.when(operations.current()).thenReturn(initialMetadata);
+ Mockito.when(catalog.loadTable(Mockito.any(TableIdentifier.class))).thenReturn(table);
+
+ adapter = new HMSCatalogAdapter("test_catalog", catalog, null, Collections.emptyList());
+
+ response = Mockito.mock(HttpServletResponse.class);
+ stringWriter = new StringWriter();
+ Mockito.when(response.getWriter()).thenReturn(new PrintWriter(stringWriter));
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (adapter != null) {
+ adapter.close();
+ }
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamOmitted() throws Exception {
+ // When snapshots query parameter is omitted (null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res = adapter.execute(HTTPMethod.GET, TABLE_PATH, null, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots including historical ones");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamNull() throws Exception {
+ // When query params map is empty (snapshots is null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, Collections.emptyMap(), null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots when query parameter map has no snapshots key");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testSnapshotModeAll() throws Exception {
+ // Standard query parameter: snapshots=all
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "all");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2, res.tableMetadata().snapshots().size(), "Mode ALL should return all snapshots");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(1L), "Historical snapshot 1 should be present");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be present");
+ }
+
+ @Test
+ void testSnapshotModeRefs() throws Exception {
+ // Standard query parameter: snapshots=refs
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "refs");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 1,
+ res.tableMetadata().snapshots().size(),
+ "Mode REFS should suppress historical snapshots");
+ Assertions.assertNull(
+ res.tableMetadata().snapshot(1L),
+ "Historical snapshot 1 should be suppressed in REFS mode");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be retained in REFS mode");
+ }
+
+ @Test
+ void testNegativeInvalidSnapshotMode() throws Exception {
Review Comment:
ACK
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6117881536
> > > @Aggarwal-Raghav please review sonar issues, seem related to the current changes
> >
> >
> > @deniskuzZ , Need 1 suggestion from you on SonaType,
> >
> > 1. HiveCatalog.java : Its in sync (code wise not formatting wise 😅) with upstream iceberg — NIT , let me know if we want to make this small change.
>
> i would keep the iceberg version in that case, easier later to sync
>
> > 2. TestHiveIcebergCRUD.java – testReadAndWriteFormatV2Partitioned_PosDelete_**RowSupplied**, testReadAndWriteFormatV2Partitioned_PosDelete_**RowNotSupplied** are using row() and as test name suggest it is testing that functionality. I'll remove these test ?
>
> could you please test if we would be able to consume legacy v2 with row data. do not need tests for write PosDelete_**RowSupplied**
I've replaced the older UT with a new UT `testConsumeLegacyFormatV2Partitioned_PosDelete_RowSupplied` , It makes use of `FileHelpers.testOnlyPosDeleteFileWithRow` (a test class from upstream iceberg, which pre-exist in hive) to generate positional deletes with rows
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245313904
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -66,45 +81,68 @@ public void appendToTable(DataFile... dataFiles) {
append.commit();
}
+ public void appendToTable(DataFile... dataFiles) {
+ appendToTable(null, dataFiles);
+ }
+
public void appendToTable(List<Record> records) throws IOException {
- appendToTable(null, records);
+ appendToTable(null, null, records);
+ }
+
+ public void appendToTable(String branch, List<Record> records) throws IOException {
+ appendToTable(null, branch, records);
+ }
+
+ public void appendToTable(StructLike partition, String branch, List<Record> records)
+ throws IOException {
+ appendToTable(branch, writeFile(partition, records));
}
public void appendToTable(StructLike partition, List<Record> records) throws IOException {
appendToTable(writeFile(partition, records));
}
+ public DataFile writeFile(List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = temp.resolve("generic-appender-test-" + UUID.randomUUID()).toFile();
+ return appendToLocalFile(table, file, fileFormat, null, records, conf);
+ }
+
public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
- File file = tmp.newFile();
- Assert.assertTrue(file.delete());
+ File file =
+ temp.resolve("generic-appender-partition-test-" + UUID.randomUUID()).toFile();
return appendToLocalFile(table, file, fileFormat, partition, records, conf);
}
- private static DataFile appendToLocalFile(Table table, File file, FileFormat format, StructLike partition,
- List<Record> records, Configuration conf)
+ private static DataFile appendToLocalFile(
+ Table table,
+ File file,
+ FileFormat format,
+ StructLike partition,
+ List<Record> records,
+ Configuration conf)
throws IOException {
- GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory.Builder builder =
+ new GenericFileWriterFactory.Builder(table).dataFileFormat(format);
+
+ // Push down ORC related settings to appender if there are any
Review Comment:
Push down ORC and Parquet ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6049770160
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4223047631
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -335,8 +335,8 @@ static void verifyMetadataHash(TableMetadata metadata, String metadataHashFromHM
}
private static byte[] hashOf(TableMetadata tableMetadata) {
- try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8)) {
- JsonGenerator generator = JsonUtil.factory().createGenerator(hashWriter);
+ try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8);
Review Comment:
Actually, upstream Iceberg also hardcodes the string "SHA-256" directly in their version of HMSTablePropertyHelper
https://github.com/apache/iceberg/blob/20d616d6a1e5c2f3e7553a2f7312330039fe3a81/hive-metastore/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java#L298-L301
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241229889
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = rowDataTemplate;
Review Comment:
does `rowDataTemplate` come from iceberg base writer?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6108728545
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241603986
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = rowDataTemplate;
Review Comment:
ok, make sense. i didn't know that `rowDataTemplate` is modified in-place.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222791283
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -84,27 +84,23 @@ public DataFile writeFile(StructLike partition, List<Record> records) throws IOE
private static DataFile appendToLocalFile(Table table, File file, FileFormat format, StructLike partition,
List<Record> records, Configuration conf)
throws IOException {
- GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table).dataFileFormat(format);
// Push down ORC related settings to appender
if (FileFormat.ORC.equals(format)) {
- appenderFactory.setAll(conf.getValByRegex(ORC_CONFIG_PREFIX));
+ builder.writerProperties(conf.getValByRegex(ORC_CONFIG_PREFIX));
}
- FileAppender<Record> appender = appenderFactory.newAppender(
- Files.localOutput(file), format);
- try (FileAppender<Record> fileAppender = appender) {
- fileAppender.addAll(records);
+ FileWriterFactory<Record> writerFactory = builder.build();
+ DataWriter<Record> appender = writerFactory.newDataWriter(
+ table.encryption().encrypt(Files.localOutput(file)), table.spec(), partition);
+ try (appender) {
+ for (Record r : records) {
Review Comment:
I can do that, I don't have any concern with that, its just upstream iceberg has the same code as we have, Is it ok to diverge?
https://github.com/apache/iceberg/blob/213a4b557db8c0a9b1eea90b908a4e6f3b1cbdf5/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java#L128-L134
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222778611
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -246,28 +246,14 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
boolean newTable = base == null;
encryptionPropsFromMetadata(metadata.properties());
boolean keepHiveStats = conf.getBoolean(ConfigProperties.KEEP_HIVE_STATS, false);
- final TableMetadata tableMetadata;
- EncryptionManager encrManager = encryption();
- if (encrManager instanceof StandardEncryptionManager) {
Review Comment:
encryption is gone or moved?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222625289
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -139,40 +140,33 @@ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
public static DataFile writeDataFile(Table table, OutputFile out, List<Record> rows) throws IOException {
FileFormat format = defaultFormat(table.properties());
- GenericAppenderFactory factory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory factory = new GenericFileWriterFactory.Builder(table).dataFileFormat(format).build();
- FileAppender<Record> writer = factory.newAppender(out, format);
- try (Closeable toClose = writer) {
- writer.addAll(rows);
+ DataWriter<Record> writer = factory.newDataWriter(encrypt(out), table.spec(), null);
+ try (writer) {
+ for (Record row : rows) {
+ writer.write(row);
+ }
}
- return DataFiles.builder(table.spec())
- .withFormat(format)
- .withPath(out.location())
- .withFileSizeInBytes(writer.length())
- .withSplitOffsets(writer.splitOffsets())
- .withMetrics(writer.metrics())
- .build();
+ return writer.toDataFile();
+
}
public static DataFile writeDataFile(Table table, OutputFile out, StructLike partition, List<Record> rows)
throws IOException {
FileFormat format = defaultFormat(table.properties());
- GenericAppenderFactory factory = new GenericAppenderFactory(table.schema(), table.spec());
+ GenericFileWriterFactory factory = new GenericFileWriterFactory.Builder(table).dataFileFormat(format).build();
- FileAppender<Record> writer = factory.newAppender(out, format);
- try (Closeable toClose = writer) {
- writer.addAll(rows);
+ DataWriter<Record> writer = factory.newDataWriter(encrypt(out), table.spec(), partition);
+ try (writer) {
+ for (Record row : rows) {
Review Comment:
same
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6138552923
Build is green.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245317456
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
Ok, so, FileHelpers and GenericAppenderHelper are sync fully from upstream
Attaching URL for your reference:
1. GenericAppenderHelper: https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
2. https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
I'll try to removed them if possible and replace them with iceberg-data test scope dependecy in a follow-up PR
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222937719
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
+ .dataFileFormat(dataFileFormat)
+ .dataSchema(dataSchema)
+ .dataSortOrder(dataSortOrder)
+ .deleteFileFormat(deleteFileFormat)
+ .equalityFieldIds(equalityFieldIds)
+ .equalityDeleteRowSchema(equalityDeleteRowSchema)
+ .equalityDeleteSortOrder(equalityDeleteSortOrder);
+
+ this.delegate = builder.build();
}
static Builder builderFor(Table table) {
return new Builder(table);
}
@Override
- protected void configureDataWrite(Avro.DataWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureEqualityDelete(Avro.DeleteWriteBuilder builder) {
-
- }
-
- @Override
- protected void configurePositionDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureDataWrite(Parquet.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
- // Configure variant shredding if enabled and a sample record is available
- if (VariantUtil.shouldUseVariantShredding(properties, dataSchema())) {
- builder.variantShreddingFunc(VariantUtil.variantShreddingFunc(sampleRecord, dataSchema()));
+ public DataWriter<Record> newDataWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition) {
+ if (dataFileFormat == FileFormat.PARQUET && VariantUtil.shouldUseVariantShredding(properties, dataSchema)) {
+ try {
+ return Parquet.writeData(file)
Review Comment:
GenericFileWriterFactory extends RegistryBasedFileWriterFactory and it uses `FormatModelRegistry.dataWriteBuilder` which returns a generic `FileWriterBuilder<DataWriter<T>, S>`
https://github.com/apache/iceberg/blob/773907d28b46f984698bb85b52b3de47053063cd/data/src/main/java/org/apache/iceberg/data/RegistryBasedFileWriterFactory.java#L107-L109
FileWriterBuilder interface does not expose a variantShreddingFunc directly.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222961746
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -19,23 +19,33 @@
package org.apache.iceberg.mr.hive.writer;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.util.Map;
import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.MetricsConfig;
+import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.StructLike;
import org.apache.iceberg.Table;
-import org.apache.iceberg.avro.Avro;
-import org.apache.iceberg.data.BaseFileWriterFactory;
+import org.apache.iceberg.data.GenericFileWriterFactory;
import org.apache.iceberg.data.Record;
-import org.apache.iceberg.data.avro.DataWriter;
-import org.apache.iceberg.data.orc.GenericOrcWriter;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
-import org.apache.iceberg.orc.ORC;
+import org.apache.iceberg.deletes.EqualityDeleteWriter;
+import org.apache.iceberg.deletes.PositionDeleteWriter;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.VariantUtil;
-class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
+class HiveFileWriterFactory implements FileWriterFactory<Record> {
Review Comment:
my bad, sorry
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222972269
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -335,8 +335,8 @@ static void verifyMetadataHash(TableMetadata metadata, String metadataHashFromHM
}
private static byte[] hashOf(TableMetadata tableMetadata) {
- try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8)) {
- JsonGenerator generator = JsonUtil.factory().createGenerator(hashWriter);
+ try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8);
Review Comment:
doesn't iceberg or existing hash util provide constant for that?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222721153
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -19,23 +19,33 @@
package org.apache.iceberg.mr.hive.writer;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.util.Map;
import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.MetricsConfig;
+import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.StructLike;
import org.apache.iceberg.Table;
-import org.apache.iceberg.avro.Avro;
-import org.apache.iceberg.data.BaseFileWriterFactory;
+import org.apache.iceberg.data.GenericFileWriterFactory;
import org.apache.iceberg.data.Record;
-import org.apache.iceberg.data.avro.DataWriter;
-import org.apache.iceberg.data.orc.GenericOrcWriter;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
-import org.apache.iceberg.orc.ORC;
+import org.apache.iceberg.deletes.EqualityDeleteWriter;
+import org.apache.iceberg.deletes.PositionDeleteWriter;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.VariantUtil;
-class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
+class HiveFileWriterFactory implements FileWriterFactory<Record> {
Review Comment:
BaseFileWriterFactory was removed?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222612299
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -84,27 +84,23 @@ public DataFile writeFile(StructLike partition, List<Record> records) throws IOE
private static DataFile appendToLocalFile(Table table, File file, FileFormat format, StructLike partition,
List<Record> records, Configuration conf)
throws IOException {
- GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table).dataFileFormat(format);
// Push down ORC related settings to appender
if (FileFormat.ORC.equals(format)) {
- appenderFactory.setAll(conf.getValByRegex(ORC_CONFIG_PREFIX));
+ builder.writerProperties(conf.getValByRegex(ORC_CONFIG_PREFIX));
}
- FileAppender<Record> appender = appenderFactory.newAppender(
- Files.localOutput(file), format);
- try (FileAppender<Record> fileAppender = appender) {
- fileAppender.addAll(records);
+ FileWriterFactory<Record> writerFactory = builder.build();
+ DataWriter<Record> appender = writerFactory.newDataWriter(
+ table.encryption().encrypt(Files.localOutput(file)), table.spec(), partition);
+ try (appender) {
+ for (Record r : records) {
Review Comment:
maybe use for-each ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241247906
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -246,28 +246,14 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
boolean newTable = base == null;
encryptionPropsFromMetadata(metadata.properties());
boolean keepHiveStats = conf.getBoolean(ConfigProperties.KEEP_HIVE_STATS, false);
- final TableMetadata tableMetadata;
- EncryptionManager encrManager = encryption();
- if (encrManager instanceof StandardEncryptionManager) {
Review Comment:
ok
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6063484177
CC @deniskuzZ
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6066808248
Thanks for handling the iceberg upgrade, @Aggarwal-Raghav ! I've added a few comments/ questions.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6111063448
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6856&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6856&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6856&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6856)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ merged PR #6856:
URL: https://github.com/apache/hive/pull/6856
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4246139066
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
ok
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245284613
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
do we need this helper with hardcoded V2?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222651428
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergAcidUtil.java:
##########
@@ -138,7 +138,8 @@ public static PositionDelete<Record> getPositionDelete(Record rec, Record rowDat
rowData.set(i - dataOffset, rec.get(i));
}
- positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L), rowData);
+ positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L));
+ positionDelete.set(2, rowData);
Review Comment:
what is this magic 2? didn't they dropped rowdata support in 1.12?
https://daily.dev/posts/decision-path-for-position-deletes-with-row-data-during-an-iceberg-1-12-upgrade-vvndcyiia
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222751927
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
+ .dataFileFormat(dataFileFormat)
+ .dataSchema(dataSchema)
+ .dataSortOrder(dataSortOrder)
+ .deleteFileFormat(deleteFileFormat)
+ .equalityFieldIds(equalityFieldIds)
+ .equalityDeleteRowSchema(equalityDeleteRowSchema)
+ .equalityDeleteSortOrder(equalityDeleteSortOrder);
+
+ this.delegate = builder.build();
}
static Builder builderFor(Table table) {
return new Builder(table);
}
@Override
- protected void configureDataWrite(Avro.DataWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureEqualityDelete(Avro.DeleteWriteBuilder builder) {
-
- }
-
- @Override
- protected void configurePositionDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureDataWrite(Parquet.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
- // Configure variant shredding if enabled and a sample record is available
- if (VariantUtil.shouldUseVariantShredding(properties, dataSchema())) {
- builder.variantShreddingFunc(VariantUtil.variantShreddingFunc(sampleRecord, dataSchema()));
+ public DataWriter<Record> newDataWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition) {
+ if (dataFileFormat == FileFormat.PARQUET && VariantUtil.shouldUseVariantShredding(properties, dataSchema)) {
Review Comment:
Yes. Will do
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222734031
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -19,23 +19,33 @@
package org.apache.iceberg.mr.hive.writer;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.util.Map;
import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.MetricsConfig;
+import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.StructLike;
import org.apache.iceberg.Table;
-import org.apache.iceberg.avro.Avro;
-import org.apache.iceberg.data.BaseFileWriterFactory;
+import org.apache.iceberg.data.GenericFileWriterFactory;
import org.apache.iceberg.data.Record;
-import org.apache.iceberg.data.avro.DataWriter;
-import org.apache.iceberg.data.orc.GenericOrcWriter;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
-import org.apache.iceberg.orc.ORC;
+import org.apache.iceberg.deletes.EqualityDeleteWriter;
+import org.apache.iceberg.deletes.PositionDeleteWriter;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.parquet.VariantUtil;
-class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
+class HiveFileWriterFactory implements FileWriterFactory<Record> {
Review Comment:
yes, https://github.com/apache/iceberg/pull/17696, mentioned in PR description
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222732877
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
Review Comment:
is this a factoryBuilder? and delegate is a writerfactory (i.e GenericFileWriterFactory)?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222801116
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -335,8 +335,8 @@ static void verifyMetadataHash(TableMetadata metadata, String metadataHashFromHM
}
private static byte[] hashOf(TableMetadata tableMetadata) {
- try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8)) {
- JsonGenerator generator = JsonUtil.factory().createGenerator(hashWriter);
+ try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8);
Review Comment:
I can introduce one, as of now it doesn't exist
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222950126
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -84,27 +84,23 @@ public DataFile writeFile(StructLike partition, List<Record> records) throws IOE
private static DataFile appendToLocalFile(Table table, File file, FileFormat format, StructLike partition,
List<Record> records, Configuration conf)
throws IOException {
- GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema());
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table).dataFileFormat(format);
// Push down ORC related settings to appender
if (FileFormat.ORC.equals(format)) {
- appenderFactory.setAll(conf.getValByRegex(ORC_CONFIG_PREFIX));
+ builder.writerProperties(conf.getValByRegex(ORC_CONFIG_PREFIX));
}
- FileAppender<Record> appender = appenderFactory.newAppender(
- Files.localOutput(file), format);
- try (FileAppender<Record> fileAppender = appender) {
- fileAppender.addAll(records);
+ FileWriterFactory<Record> writerFactory = builder.build();
+ DataWriter<Record> appender = writerFactory.newDataWriter(
+ table.encryption().encrypt(Files.localOutput(file)), table.spec(), partition);
+ try (appender) {
+ for (Record r : records) {
Review Comment:
oh, i didn't know. i thought we only have copy for the catalog - disregard then
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4246139066
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
ok, i'll merge once we get a green build. Thanks again for addressing the review comments!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241228899
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java:
##########
@@ -378,7 +379,12 @@ private RESTResponse tableExists(Map<String, String> vars) {
private LoadTableResponse loadTable(Map<String, String> vars) {
TableIdentifier ident = identFromPathVars(vars);
- return CatalogHandlers.loadTable(catalog, ident);
+ String snapshots = vars.get(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER);
+ RESTCatalogProperties.SnapshotMode mode = RESTCatalogProperties.SNAPSHOT_LOADING_MODE_DEFAULT;
+ if (snapshots != null) {
+ mode = RESTCatalogProperties.SnapshotMode.valueOf(snapshots.toUpperCase(Locale.ROOT));
Review Comment:
do we have any tests for that?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245390283
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCatalogAdapterSnapshotMode.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.iceberg.BaseTable;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableOperations;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestHMSCatalogAdapterSnapshotMode {
+
+ private static final String TABLE_PATH = "v1/namespaces/accounting/tables/my_table";
+
+ private HMSCatalogAdapter adapter;
+ private HttpServletResponse response;
+ private StringWriter stringWriter;
+ private TableMetadata initialMetadata;
+
+ @BeforeEach
+ void setup() throws Exception {
+ Catalog catalog =
+ Mockito.mock(
+ Catalog.class,
+ Mockito.withSettings().extraInterfaces(SupportsNamespaces.class, ViewCatalog.class));
+
+ Schema schema = new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ Snapshot snap1 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap1.snapshotId()).thenReturn(1L);
+ Mockito.when(snap1.sequenceNumber()).thenReturn(1L);
+ Mockito.when(snap1.timestampMillis()).thenReturn(1000L);
+
+ Snapshot snap2 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap2.snapshotId()).thenReturn(2L);
+ Mockito.when(snap2.sequenceNumber()).thenReturn(2L);
+ Mockito.when(snap2.timestampMillis()).thenReturn(2000L);
+
+ initialMetadata =
+ TableMetadata.buildFromEmpty(2)
+ .setLocation("file:///tmp/table")
+ .addSchema(schema)
+ .setDefaultPartitionSpec(PartitionSpec.unpartitioned())
+ .setDefaultSortOrder(SortOrder.unsorted())
+ .addSnapshot(snap1)
+ .addSnapshot(snap2)
+ .setBranchSnapshot(2L, SnapshotRef.MAIN_BRANCH)
+ .discardChanges()
+ .withMetadataLocation("file:///tmp/table/metadata.json")
+ .build();
+
+ BaseTable table = Mockito.mock(BaseTable.class);
+ TableOperations operations = Mockito.mock(TableOperations.class);
+ Mockito.when(table.operations()).thenReturn(operations);
+ Mockito.when(operations.current()).thenReturn(initialMetadata);
+ Mockito.when(catalog.loadTable(Mockito.any(TableIdentifier.class))).thenReturn(table);
+
+ adapter = new HMSCatalogAdapter("test_catalog", catalog, null, Collections.emptyList());
+
+ response = Mockito.mock(HttpServletResponse.class);
+ stringWriter = new StringWriter();
+ Mockito.when(response.getWriter()).thenReturn(new PrintWriter(stringWriter));
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (adapter != null) {
+ adapter.close();
+ }
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamOmitted() throws Exception {
+ // When snapshots query parameter is omitted (null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res = adapter.execute(HTTPMethod.GET, TABLE_PATH, null, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots including historical ones");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamNull() throws Exception {
+ // When query params map is empty (snapshots is null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, Collections.emptyMap(), null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots when query parameter map has no snapshots key");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testSnapshotModeAll() throws Exception {
+ // Standard query parameter: snapshots=all
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "all");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2, res.tableMetadata().snapshots().size(), "Mode ALL should return all snapshots");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(1L), "Historical snapshot 1 should be present");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be present");
+ }
+
+ @Test
+ void testSnapshotModeRefs() throws Exception {
+ // Standard query parameter: snapshots=refs
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "refs");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 1,
+ res.tableMetadata().snapshots().size(),
+ "Mode REFS should suppress historical snapshots");
+ Assertions.assertNull(
+ res.tableMetadata().snapshot(1L),
+ "Historical snapshot 1 should be suppressed in REFS mode");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be retained in REFS mode");
+ }
+
+ @Test
+ void testNegativeInvalidSnapshotMode() throws Exception {
+ // Unknown mode value: snapshots=invalid
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "invalid");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for an invalid snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ String responseBody = stringWriter.toString();
+ Assertions.assertTrue(
+ responseBody.contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException");
+ Assertions.assertTrue(
+ responseBody.contains("INVALID"), "Error response message should mention the invalid mode");
+ }
+
+ @Test
+ void testNegativeEmptySnapshotMode() throws Exception {
+ // Empty string value: snapshots=""
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for empty snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ Assertions.assertTrue(
+ stringWriter.toString().contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException for empty mode");
+ }
+
+ @Test
+ void testNegativeNumericSnapshotMode() throws Exception {
Review Comment:
sure let me check.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245358028
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCatalogAdapterSnapshotMode.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.iceberg.BaseTable;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableOperations;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestHMSCatalogAdapterSnapshotMode {
+
+ private static final String TABLE_PATH = "v1/namespaces/accounting/tables/my_table";
+
+ private HMSCatalogAdapter adapter;
+ private HttpServletResponse response;
+ private StringWriter stringWriter;
+ private TableMetadata initialMetadata;
+
+ @BeforeEach
+ void setup() throws Exception {
+ Catalog catalog =
+ Mockito.mock(
+ Catalog.class,
+ Mockito.withSettings().extraInterfaces(SupportsNamespaces.class, ViewCatalog.class));
+
+ Schema schema = new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ Snapshot snap1 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap1.snapshotId()).thenReturn(1L);
+ Mockito.when(snap1.sequenceNumber()).thenReturn(1L);
+ Mockito.when(snap1.timestampMillis()).thenReturn(1000L);
+
+ Snapshot snap2 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap2.snapshotId()).thenReturn(2L);
+ Mockito.when(snap2.sequenceNumber()).thenReturn(2L);
+ Mockito.when(snap2.timestampMillis()).thenReturn(2000L);
+
+ initialMetadata =
+ TableMetadata.buildFromEmpty(2)
+ .setLocation("file:///tmp/table")
+ .addSchema(schema)
+ .setDefaultPartitionSpec(PartitionSpec.unpartitioned())
+ .setDefaultSortOrder(SortOrder.unsorted())
+ .addSnapshot(snap1)
+ .addSnapshot(snap2)
+ .setBranchSnapshot(2L, SnapshotRef.MAIN_BRANCH)
+ .discardChanges()
+ .withMetadataLocation("file:///tmp/table/metadata.json")
+ .build();
+
+ BaseTable table = Mockito.mock(BaseTable.class);
+ TableOperations operations = Mockito.mock(TableOperations.class);
+ Mockito.when(table.operations()).thenReturn(operations);
+ Mockito.when(operations.current()).thenReturn(initialMetadata);
+ Mockito.when(catalog.loadTable(Mockito.any(TableIdentifier.class))).thenReturn(table);
+
+ adapter = new HMSCatalogAdapter("test_catalog", catalog, null, Collections.emptyList());
+
+ response = Mockito.mock(HttpServletResponse.class);
+ stringWriter = new StringWriter();
+ Mockito.when(response.getWriter()).thenReturn(new PrintWriter(stringWriter));
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (adapter != null) {
+ adapter.close();
+ }
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamOmitted() throws Exception {
+ // When snapshots query parameter is omitted (null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res = adapter.execute(HTTPMethod.GET, TABLE_PATH, null, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots including historical ones");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamNull() throws Exception {
+ // When query params map is empty (snapshots is null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, Collections.emptyMap(), null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots when query parameter map has no snapshots key");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testSnapshotModeAll() throws Exception {
+ // Standard query parameter: snapshots=all
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "all");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2, res.tableMetadata().snapshots().size(), "Mode ALL should return all snapshots");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(1L), "Historical snapshot 1 should be present");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be present");
+ }
+
+ @Test
+ void testSnapshotModeRefs() throws Exception {
+ // Standard query parameter: snapshots=refs
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "refs");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 1,
+ res.tableMetadata().snapshots().size(),
+ "Mode REFS should suppress historical snapshots");
+ Assertions.assertNull(
+ res.tableMetadata().snapshot(1L),
+ "Historical snapshot 1 should be suppressed in REFS mode");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be retained in REFS mode");
+ }
+
+ @Test
+ void testNegativeInvalidSnapshotMode() throws Exception {
Review Comment:
negative is confusing here. maybe simply testInvalidSnapshotMode
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245389817
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
ok, meanwhile if some introduced methods are dead- can we kill them?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245317456
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
Ok, so, FileHelpers and GenericAppenderHelper are sync fully from upstream
Attaching URL for your reference:
1. GenericAppenderHelper: https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
2. https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
I'll try to removed them if possible and replace them with iceberg-data test scope dependecy in a follow-up PR
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245366484
##########
standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCatalogAdapterSnapshotMode.java:
##########
@@ -0,0 +1,268 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.Map;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.iceberg.BaseTable;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableOperations;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.SupportsNamespaces;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.catalog.ViewCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestHMSCatalogAdapterSnapshotMode {
+
+ private static final String TABLE_PATH = "v1/namespaces/accounting/tables/my_table";
+
+ private HMSCatalogAdapter adapter;
+ private HttpServletResponse response;
+ private StringWriter stringWriter;
+ private TableMetadata initialMetadata;
+
+ @BeforeEach
+ void setup() throws Exception {
+ Catalog catalog =
+ Mockito.mock(
+ Catalog.class,
+ Mockito.withSettings().extraInterfaces(SupportsNamespaces.class, ViewCatalog.class));
+
+ Schema schema = new Schema(Types.NestedField.required(1, "id", Types.IntegerType.get()));
+
+ Snapshot snap1 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap1.snapshotId()).thenReturn(1L);
+ Mockito.when(snap1.sequenceNumber()).thenReturn(1L);
+ Mockito.when(snap1.timestampMillis()).thenReturn(1000L);
+
+ Snapshot snap2 = Mockito.mock(Snapshot.class);
+ Mockito.when(snap2.snapshotId()).thenReturn(2L);
+ Mockito.when(snap2.sequenceNumber()).thenReturn(2L);
+ Mockito.when(snap2.timestampMillis()).thenReturn(2000L);
+
+ initialMetadata =
+ TableMetadata.buildFromEmpty(2)
+ .setLocation("file:///tmp/table")
+ .addSchema(schema)
+ .setDefaultPartitionSpec(PartitionSpec.unpartitioned())
+ .setDefaultSortOrder(SortOrder.unsorted())
+ .addSnapshot(snap1)
+ .addSnapshot(snap2)
+ .setBranchSnapshot(2L, SnapshotRef.MAIN_BRANCH)
+ .discardChanges()
+ .withMetadataLocation("file:///tmp/table/metadata.json")
+ .build();
+
+ BaseTable table = Mockito.mock(BaseTable.class);
+ TableOperations operations = Mockito.mock(TableOperations.class);
+ Mockito.when(table.operations()).thenReturn(operations);
+ Mockito.when(operations.current()).thenReturn(initialMetadata);
+ Mockito.when(catalog.loadTable(Mockito.any(TableIdentifier.class))).thenReturn(table);
+
+ adapter = new HMSCatalogAdapter("test_catalog", catalog, null, Collections.emptyList());
+
+ response = Mockito.mock(HttpServletResponse.class);
+ stringWriter = new StringWriter();
+ Mockito.when(response.getWriter()).thenReturn(new PrintWriter(stringWriter));
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (adapter != null) {
+ adapter.close();
+ }
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamOmitted() throws Exception {
+ // When snapshots query parameter is omitted (null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res = adapter.execute(HTTPMethod.GET, TABLE_PATH, null, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots including historical ones");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testDefaultSnapshotModeWhenParamNull() throws Exception {
+ // When query params map is empty (snapshots is null), mode defaults to SnapshotMode.ALL
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, Collections.emptyMap(), null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2,
+ res.tableMetadata().snapshots().size(),
+ "Default mode (ALL) should preserve all snapshots when query parameter map has no snapshots key");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(1L), "Snapshot 1 should be retained");
+ Assertions.assertNotNull(res.tableMetadata().snapshot(2L), "Snapshot 2 should be retained");
+ }
+
+ @Test
+ void testSnapshotModeAll() throws Exception {
+ // Standard query parameter: snapshots=all
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "all");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 2, res.tableMetadata().snapshots().size(), "Mode ALL should return all snapshots");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(1L), "Historical snapshot 1 should be present");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be present");
+ }
+
+ @Test
+ void testSnapshotModeRefs() throws Exception {
+ // Standard query parameter: snapshots=refs
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "refs");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNotNull(res, "Response should not be null");
+ Assertions.assertNotNull(res.tableMetadata(), "TableMetadata should not be null");
+ Assertions.assertEquals(
+ 1,
+ res.tableMetadata().snapshots().size(),
+ "Mode REFS should suppress historical snapshots");
+ Assertions.assertNull(
+ res.tableMetadata().snapshot(1L),
+ "Historical snapshot 1 should be suppressed in REFS mode");
+ Assertions.assertNotNull(
+ res.tableMetadata().snapshot(2L), "Branch snapshot 2 should be retained in REFS mode");
+ }
+
+ @Test
+ void testNegativeInvalidSnapshotMode() throws Exception {
+ // Unknown mode value: snapshots=invalid
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "invalid");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for an invalid snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ String responseBody = stringWriter.toString();
+ Assertions.assertTrue(
+ responseBody.contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException");
+ Assertions.assertTrue(
+ responseBody.contains("INVALID"), "Error response message should mention the invalid mode");
+ }
+
+ @Test
+ void testNegativeEmptySnapshotMode() throws Exception {
+ // Empty string value: snapshots=""
+ Map<String, String> queryParams =
+ ImmutableMap.of(RESTCatalogProperties.SNAPSHOTS_QUERY_PARAMETER, "");
+ LoadTableResponse res =
+ adapter.execute(HTTPMethod.GET, TABLE_PATH, queryParams, null, response);
+
+ Assertions.assertNull(res, "Response should be null for empty snapshot mode");
+ Mockito.verify(response).setStatus(400);
+ Assertions.assertTrue(
+ stringWriter.toString().contains("IllegalArgumentException"),
+ "Error response should report IllegalArgumentException for empty mode");
+ }
+
+ @Test
+ void testNegativeNumericSnapshotMode() throws Exception {
Review Comment:
please check all these test-cases are actually neede, maybe extract common parts
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245389817
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
ok
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245307480
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java:
##########
@@ -66,45 +81,68 @@ public void appendToTable(DataFile... dataFiles) {
append.commit();
}
+ public void appendToTable(DataFile... dataFiles) {
+ appendToTable(null, dataFiles);
+ }
+
public void appendToTable(List<Record> records) throws IOException {
- appendToTable(null, records);
+ appendToTable(null, null, records);
+ }
+
+ public void appendToTable(String branch, List<Record> records) throws IOException {
+ appendToTable(null, branch, records);
+ }
+
+ public void appendToTable(StructLike partition, String branch, List<Record> records)
+ throws IOException {
+ appendToTable(branch, writeFile(partition, records));
}
public void appendToTable(StructLike partition, List<Record> records) throws IOException {
appendToTable(writeFile(partition, records));
}
+ public DataFile writeFile(List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = temp.resolve("generic-appender-test-" + UUID.randomUUID()).toFile();
+ return appendToLocalFile(table, file, fileFormat, null, records, conf);
+ }
+
public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
- File file = tmp.newFile();
- Assert.assertTrue(file.delete());
+ File file =
+ temp.resolve("generic-appender-partition-test-" + UUID.randomUUID()).toFile();
Review Comment:
could you please moveto constant "generic-appender-partition-test-"
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4245317456
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java:
##########
@@ -38,159 +40,180 @@
import org.apache.iceberg.encryption.EncryptedFiles;
import org.apache.iceberg.encryption.EncryptedOutputFile;
import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.io.DataWriter;
+import org.apache.iceberg.io.DeleteSchemaUtil;
import org.apache.iceberg.io.FileAppender;
-import org.apache.iceberg.io.FileAppenderFactory;
import org.apache.iceberg.io.FileWriterFactory;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.parquet.Parquet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.Pair;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT;
-import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT;
-
public class FileHelpers {
private FileHelpers() {
}
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out,
- List<Pair<CharSequence, Long>> deletes)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes);
- }
-
- public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Pair<CharSequence, Long>> deletes)
+ public static DeleteFile testOnlyPosDeleteFileWithRow(
+ Table table, OutputFile out, StructLike partition, List<Record> posDeleteRecords)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec());
+ Schema posDeleteSchema = DeleteSchemaUtil.posDeleteSchema(table.schema());
+ FileAppender<Record> appender =
+ Parquet.write(out)
+ .schema(posDeleteSchema)
+ .createWriterFunc(GenericParquetWriter::create)
+ .metricsConfig(MetricsConfig.forPositionDelete())
+ .overwrite()
+ .build();
+ try (Closeable ignored = appender) {
+ posDeleteRecords.forEach(appender::add);
+ }
- PositionDeleteWriter<Record> writer =
- factory.newPosDeleteWriter(encrypt(out), format, partition);
- PositionDelete<Record> posDelete = PositionDelete.create();
- try (Closeable toClose = writer) {
- for (Pair<CharSequence, Long> delete : deletes) {
- writer.write(posDelete.set(delete.first(), delete.second(), null));
- }
+ FileMetadata.Builder builder =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PARQUET)
+ .withPath(out.location())
+ .withFileSizeInBytes(appender.length())
+ .withMetrics(appender.metrics());
+ if (partition != null) {
+ builder.withPartition(partition);
}
- return Pair.of(writer.toDeleteFile(), writer.referencedDataFiles());
+ return builder.build();
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, List<Record> deletes, Schema deleteRowSchema)
- throws IOException {
- return writeDeleteFile(table, out, null, deletes, deleteRowSchema);
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes) throws IOException {
+ return writeDeleteFile(table, out, null, deletes);
}
- public static DeleteFile writeDeleteFile(Table table, OutputFile out, StructLike partition,
- List<Record> deletes, Schema deleteRowSchema)
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
+ Table table, OutputFile out, List<Pair<CharSequence, Long>> deletes, int formatVersion)
throws IOException {
- FileFormat format = defaultFormat(table.properties());
- int[] equalityFieldIds = deleteRowSchema.columns().stream().mapToInt(Types.NestedField::fieldId).toArray();
- FileAppenderFactory<Record> factory = new GenericAppenderFactory(table.schema(), table.spec(),
- equalityFieldIds, deleteRowSchema, null);
-
- EqualityDeleteWriter<Record> writer = factory.newEqDeleteWriter(encrypt(out), format, partition);
- try (Closeable toClose = writer) {
- writer.write(deletes);
- }
+ return writeDeleteFile(table, out, null, deletes, formatVersion);
+ }
- return writer.toDeleteFile();
+ public static Pair<DeleteFile, CharSequenceSet> writeDeleteFile(
Review Comment:
Ok, so, FileHelpers and GenericAppenderHelper are sync fully from upstream
Attaching URL for your reference:
1. GenericAppenderHelper: https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
2. https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/data/src/test/java/org/apache/iceberg/data/FileHelpers.java
I'll try to removed them if possible and replace them with iceberg-data test scope dependecy.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4241248681
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -352,6 +363,37 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
Review Comment:
ok
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on PR #6856:
URL: https://github.com/apache/hive/pull/6856#issuecomment-6108858121
I've synced up the following with upstream iceberg 1.12.0
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/FileHelpers.java
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222594901
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/test/utils/HiveIcebergTestUtils.java:
##########
@@ -407,12 +410,14 @@ public static DeleteFile createPositionalDeleteFile(Table table, String deleteFi
partitionKey.partition(record);
}
- PositionDeleteWriter<Record> posWriter = appenderFactory.newPosDeleteWriter(outputFile, fileFormat, partitionKey);
- try (PositionDeleteWriter<Record> writer = posWriter) {
+ PositionDeleteWriter<Record> posWriter = writerFactory
+ .newPositionDeleteWriter(outputFile, table.spec(), partitionKey);
+ try (posWriter) {
deletes.forEach(del -> {
PositionDelete positionDelete = PositionDelete.create();
- positionDelete.set(del.path(), del.pos(), del.row());
- writer.write(positionDelete);
+ positionDelete.set(del.path(), del.pos());
+ positionDelete.set(2, del.get(2, Record.class));
Review Comment:
what is this magic 2?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222709276
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
+ .dataFileFormat(dataFileFormat)
+ .dataSchema(dataSchema)
+ .dataSortOrder(dataSortOrder)
+ .deleteFileFormat(deleteFileFormat)
+ .equalityFieldIds(equalityFieldIds)
+ .equalityDeleteRowSchema(equalityDeleteRowSchema)
+ .equalityDeleteSortOrder(equalityDeleteSortOrder);
+
+ this.delegate = builder.build();
}
static Builder builderFor(Table table) {
return new Builder(table);
}
@Override
- protected void configureDataWrite(Avro.DataWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureEqualityDelete(Avro.DeleteWriteBuilder builder) {
-
- }
-
- @Override
- protected void configurePositionDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
- }
-
- @Override
- protected void configureDataWrite(Parquet.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
- // Configure variant shredding if enabled and a sample record is available
- if (VariantUtil.shouldUseVariantShredding(properties, dataSchema())) {
- builder.variantShreddingFunc(VariantUtil.variantShreddingFunc(sampleRecord, dataSchema()));
+ public DataWriter<Record> newDataWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition) {
+ if (dataFileFormat == FileFormat.PARQUET && VariantUtil.shouldUseVariantShredding(properties, dataSchema)) {
+ try {
+ return Parquet.writeData(file)
Review Comment:
delegate.newDataWriter doesn't support shredding func arg?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222732877
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
Review Comment:
is this a factoryBuilder? and delegate is a writerfactory?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222675017
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveIcebergCopyOnWriteRecordWriter.java:
##########
@@ -55,7 +55,7 @@ class HiveIcebergCopyOnWriteRecordWriter extends HiveIcebergDefaultWriter {
public void write(Writable row) throws IOException {
Record record = ((Container<Record>) row).get();
PositionDelete<Record> positionDelete = IcebergAcidUtil.getPositionDelete(record, rowDataTemplate, false);
- Record rowData = positionDelete.row();
+ Record rowData = positionDelete.get(2, Record.class);
Review Comment:
again. if row data in deletes were removed, why we keep some leftovers. or i am missing something here?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "Aggarwal-Raghav (via GitHub)" <gi...@apache.org>.
Aggarwal-Raghav commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222673418
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergAcidUtil.java:
##########
@@ -138,7 +138,8 @@ public static PositionDelete<Record> getPositionDelete(Record rec, Record rowDat
rowData.set(i - dataOffset, rec.get(i));
}
- positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L), rowData);
+ positionDelete.set(filePath, ObjectUtils.defaultIfNull(filePosition, 0L));
+ positionDelete.set(2, rowData);
Review Comment:
they did, please check important points in PR description. I've mentioned breaking changes iceberg PR's
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222732877
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/writer/HiveFileWriterFactory.java:
##########
@@ -47,80 +57,66 @@ class HiveFileWriterFactory extends BaseFileWriterFactory<Record> {
FileFormat deleteFileFormat,
int[] equalityFieldIds,
Schema equalityDeleteRowSchema,
- SortOrder equalityDeleteSortOrder,
- Schema positionDeleteRowSchema) {
- super(
- table,
- dataFileFormat,
- dataSchema,
- dataSortOrder,
- deleteFileFormat,
- equalityFieldIds,
- equalityDeleteRowSchema,
- equalityDeleteSortOrder,
- positionDeleteRowSchema);
- properties = table.properties();
+ SortOrder equalityDeleteSortOrder) {
+ this.table = table;
+ this.dataFileFormat = dataFileFormat;
+ this.dataSchema = dataSchema;
+ this.properties = table.properties();
+
+ GenericFileWriterFactory.Builder builder = new GenericFileWriterFactory.Builder(table)
Review Comment:
is this a factoryBuilder? and delegate is a writerfactory (i.e GenericFileWriterFactory)?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222762527
##########
iceberg/iceberg-catalog/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -335,8 +335,8 @@ static void verifyMetadataHash(TableMetadata metadata, String metadataHashFromHM
}
private static byte[] hashOf(TableMetadata tableMetadata) {
- try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8)) {
- JsonGenerator generator = JsonUtil.factory().createGenerator(hashWriter);
+ try (HashWriter hashWriter = new HashWriter("SHA-256", StandardCharsets.UTF_8);
Review Comment:
is there a constant for "SHA-256" we can use?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-30127: Iceberg: Upgrade iceberg version to 1.12.0 [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #6856:
URL: https://github.com/apache/hive/pull/6856#discussion_r4222931287
##########
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/test/utils/HiveIcebergTestUtils.java:
##########
@@ -407,12 +410,14 @@ public static DeleteFile createPositionalDeleteFile(Table table, String deleteFi
partitionKey.partition(record);
}
- PositionDeleteWriter<Record> posWriter = appenderFactory.newPosDeleteWriter(outputFile, fileFormat, partitionKey);
- try (PositionDeleteWriter<Record> writer = posWriter) {
+ PositionDeleteWriter<Record> posWriter = writerFactory
+ .newPositionDeleteWriter(outputFile, table.spec(), partitionKey);
+ try (posWriter) {
deletes.forEach(del -> {
PositionDelete positionDelete = PositionDelete.create();
- positionDelete.set(del.path(), del.pos(), del.row());
- writer.write(positionDelete);
+ positionDelete.set(del.path(), del.pos());
+ positionDelete.set(2, del.get(2, Record.class));
Review Comment:
i don't think we need any workaround, we shouldn't read row data fields even if they were written before
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org