You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "1raghavmahajan (via GitHub)" <gi...@apache.org> on 2025/10/10 13:30:18 UTC
[PR] Utility method to get column reference [iceberg]
1raghavmahajan opened a new pull request, #14291:
URL: https://github.com/apache/iceberg/pull/14291
Small nit: extracted method to get column references given a list of string column names
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2428080247
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -184,16 +185,9 @@ private Dataset<Row> computeUpdateImages(String[] identifierColumns, Dataset<Row
identifierColumns.length > 0,
"Cannot compute the update images because identifier columns are not set");
- Column[] repartitionSpec = new Column[identifierColumns.length + 1];
- for (int i = 0; i < identifierColumns.length; i++) {
- repartitionSpec[i] = df.col(CreateChangelogViewProcedure.delimitedName(identifierColumns[i]));
- }
-
- repartitionSpec[repartitionSpec.length - 1] = df.col(MetadataColumns.CHANGE_ORDINAL.name());
-
- String[] identifierFields = Arrays.copyOf(identifierColumns, identifierColumns.length + 1);
- identifierFields[identifierFields.length - 1] = MetadataColumns.CHANGE_ORDINAL.name();
-
+ String[] identifierFields =
+ ArrayUtil.add(identifierColumns, MetadataColumns.CHANGE_ORDINAL.name());
+ Column[] repartitionSpec = getColumnReferences(df, identifierFields);
Review Comment:
I guess we could also just simplify this without having to introduce a separate method:
```
String[] identifierFields =
ArrayUtil.add(identifierColumns, MetadataColumns.CHANGE_ORDINAL.name());
Column[] repartitionSpec =
Arrays.stream(identifierFields)
.map(col -> df.col(CreateChangelogViewProcedure.delimitedName(col)))
.toArray(Column[]::new);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2421192085
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
This procedure is has a few places ([#1](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L189), [#2](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L304)) where it needs to build the column references for a list of identifier column strings and combine them with the metadata columns (change ordinal / change type).
This leads to a verbose piece of code where just to build a partition spec. I wasn't able to generalize the second instance, but I do think it cleans things up a bit on the callee side.
I agree with you that it can lead to confusion since it's still doing a bit too much and now we lack the context of _why_. Let me see if I can refactor this better.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Refactor to use ArrayUtils [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #14291:
URL: https://github.com/apache/iceberg/pull/14291
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2428071303
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +308,21 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ /**
+ * Retrieves the delimited column references for the given column names
+ *
+ * @param df dataframe to use for column references
+ * @param columnNames list of column names
+ * @return combined list of column references for the given 2 lists of column names
+ */
+ private static Column[] getColumnReferences(Dataset<Row> df, String[] columnNames) {
Review Comment:
nit: the codebase typically doesn't use `get` prefixes, so maybe rename this to `fetchColumns` or `fetchColumnReferences`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2420780025
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
I feel like this method makes it actually more difficult to understand what the code is doing. Also I'm not fully understanding what the second for loop is doing?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2429594312
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +308,21 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ /**
+ * Retrieves the delimited column references for the given column names
+ *
+ * @param df dataframe to use for column references
+ * @param columnNames list of column names
+ * @return combined list of column references for the given 2 lists of column names
+ */
+ private static Column[] getColumnReferences(Dataset<Row> df, String[] columnNames) {
Review Comment:
Updated https://github.com/apache/iceberg/commit/f46a28ddc189f2683493dc0a0fe4a4c5ee79e770
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2421192085
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
This procedure is has a few places ([#1](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L189), [#2](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L304)) where it needs to build the column references for a list of identifier column strings and combine them with the metadata columns (change ordinal / change type).
This leads to a slightly verbose piece of code just to build a partition spec. I wasn't able to generalize the second instance, but I do think it cleans things up a bit on the callee side.
I agree with you that it can lead to confusion since it's still doing a bit too much and now we lack the context of _why_. Let me see if I can refactor this better.
Thanks for the review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2421192085
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
This procedure is has a few places ([#1](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L189), [#2](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L304)) where it needs to build the column references for a list of identifier column strings and combine them with the metadata columns (change ordinal / change type).
This leads to a verbose piece of code where just to build a partition spec. I wasn't able to generalize the second instance, but I do think it cleans things up a bit on the callee side.
I agree with you that it can lead to confusion since it's still doing a bit too much and now we lack the context of _why_. Let me see if I can refactor this better. Thanks for the review.
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
This procedure is has a few places ([#1](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L189), [#2](https://github.com/apache/iceberg/blob/7f411135562e682916f8f411bfc7455e3f79956d/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java#L304)) where it needs to build the column references for a list of identifier column strings and combine them with the metadata columns (change ordinal / change type).
This leads to a verbose piece of code where just to build a partition spec. I wasn't able to generalize the second instance, but I do think it cleans things up a bit on the callee side.
I agree with you that it can lead to confusion since it's still doing a bit too much and now we lack the context of _why_. Let me see if I can refactor this better.
Thanks for the review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2421306799
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -314,6 +310,18 @@ private static Column[] sortSpec(Dataset<Row> df, Column[] repartitionSpec, bool
return sortSpec;
}
+ private static Column[] getColumns(Dataset<Row> df, String[] initialColumns, String... addCols) {
Review Comment:
I've updated it.
Now it's just a single loop, and we use a separate util for concatenating.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Utility method to get column reference [iceberg]
Posted by "1raghavmahajan (via GitHub)" <gi...@apache.org>.
1raghavmahajan commented on code in PR #14291:
URL: https://github.com/apache/iceberg/pull/14291#discussion_r2429593355
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/CreateChangelogViewProcedure.java:
##########
@@ -184,16 +185,9 @@ private Dataset<Row> computeUpdateImages(String[] identifierColumns, Dataset<Row
identifierColumns.length > 0,
"Cannot compute the update images because identifier columns are not set");
- Column[] repartitionSpec = new Column[identifierColumns.length + 1];
- for (int i = 0; i < identifierColumns.length; i++) {
- repartitionSpec[i] = df.col(CreateChangelogViewProcedure.delimitedName(identifierColumns[i]));
- }
-
- repartitionSpec[repartitionSpec.length - 1] = df.col(MetadataColumns.CHANGE_ORDINAL.name());
-
- String[] identifierFields = Arrays.copyOf(identifierColumns, identifierColumns.length + 1);
- identifierFields[identifierFields.length - 1] = MetadataColumns.CHANGE_ORDINAL.name();
-
+ String[] identifierFields =
+ ArrayUtil.add(identifierColumns, MetadataColumns.CHANGE_ORDINAL.name());
+ Column[] repartitionSpec = getColumnReferences(df, identifierFields);
Review Comment:
Had kept it separate as I preferred it for a [follow-up PR](https://github.com/apache/iceberg/pull/14293). Though I don't mind it being done with those changes. 👍🏼
Pushed https://github.com/apache/iceberg/pull/14291/commits/f46a28ddc189f2683493dc0a0fe4a4c5ee79e770
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org