You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "majian1998 (via GitHub)" <gi...@apache.org> on 2025/11/04 02:11:54 UTC
[PR] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
majian1998 opened a new pull request, #14497:
URL: https://github.com/apache/iceberg/pull/14497
Related PR: https://github.com/apache/iceberg/pull/13625, https://github.com/apache/iceberg/pull/14483
Some follow-up code optimizations. Thanks to @kamcheungting-db for the suggestions.
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488921600
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -42,7 +42,6 @@
import org.junit.jupiter.api.Test;
public class TestExtendedParser {
-
Review Comment:
unnecessary change?
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2492588180
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -74,12 +78,7 @@ private static <T> T findParser(ParserInterface parser, Class<T> clazz) {
return clazz.cast(current);
}
- ParserInterface next = getNextDelegateParser(current);
- if (next == null) {
- break;
- }
-
- current = next;
+ current = getNextDelegateParser(current);
Review Comment:
This just simplifies the code. The logic stays the same. It’s a follow-up PR, and the original tests already cover this.
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488812347
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -67,6 +70,10 @@ public void saveOriginalParser() throws Exception {
try {
parserField = clazz.getDeclaredField(SQL_PARSER_FIELD);
} catch (NoSuchFieldException e) {
+ LOG.warn(
+ "Field '{}' not found in class {}. Trying superclass.",
+ SQL_PARSER_FIELD,
+ clazz.getName());
Review Comment:
> @kamcheungting-db: add log to help RCA if something goes wrong
Added logs based on suggestion, just in case.
If you feel they're unnecessary in the test class, we can remove them.
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -67,6 +70,10 @@ public void saveOriginalParser() throws Exception {
try {
parserField = clazz.getDeclaredField(SQL_PARSER_FIELD);
} catch (NoSuchFieldException e) {
+ LOG.warn(
+ "Field '{}' not found in class {}. Trying superclass.",
+ SQL_PARSER_FIELD,
+ clazz.getName());
Review Comment:
> @kamcheungting-db: add log to help RCA if something goes wrong
Added logs based on suggestion, just in case.
If you feel they're unnecessary in the test class, we can remove 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: 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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2490603376
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -74,12 +78,7 @@ private static <T> T findParser(ParserInterface parser, Class<T> clazz) {
return clazz.cast(current);
}
- ParserInterface next = getNextDelegateParser(current);
- if (next == null) {
- break;
- }
-
- current = next;
+ current = getNextDelegateParser(current);
Review Comment:
is this fixing an actual issue? It would be good to have a test for this
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2489237135
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,7 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
Review Comment:
I think we can remove this comment now. This comment was there to suppress "Empty 'catch' block" warning.
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,7 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
+ LOG.warn("Failed to scan delegate parser in {}: {}", parser.getClass().getName(), e);
Review Comment:
We don't need to append `: {}` for `e`, right?
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2492773641
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -26,8 +26,12 @@
import org.apache.spark.sql.AnalysisException;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.parser.ParserInterface;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public interface ExtendedParser extends ParserInterface {
+ Logger LOG = LoggerFactory.getLogger(ExtendedParser.class);
Review Comment:
Great tip, thank you!
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488744748
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -74,18 +76,15 @@ private static <T> T findParser(ParserInterface parser, Class<T> clazz) {
return clazz.cast(current);
}
- ParserInterface next = getNextDelegateParser(current);
- if (next == null) {
- break;
- }
-
- current = next;
+ current = getNextDelegateParser(current);
}
return null;
}
private static ParserInterface getNextDelegateParser(ParserInterface parser) {
+ Logger logger = LoggerFactory.getLogger(ExtendedParser.class);
Review Comment:
can we not declare this as `private static final` member of the interface
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,8 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
+ logger.warn(
+ "Failed to scan delegate parser in {}: {}", parser.getClass().getName(), e.toString());
Review Comment:
```suggestion
logger.warn("Failed to scan delegate parser in {}", parser.getClass().getName(), e);
```
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -67,6 +70,10 @@ public void saveOriginalParser() throws Exception {
try {
parserField = clazz.getDeclaredField(SQL_PARSER_FIELD);
} catch (NoSuchFieldException e) {
+ LOG.warn(
+ "Field '{}' not found in class {}. Trying superclass.",
+ SQL_PARSER_FIELD,
+ clazz.getName());
Review Comment:
LOG lines for 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: 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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#issuecomment-3494578234
Thanks @majian1998 for the PR! Thanks everyone 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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2492754343
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -26,8 +26,12 @@
import org.apache.spark.sql.AnalysisException;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.parser.ParserInterface;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public interface ExtendedParser extends ParserInterface {
+ Logger LOG = LoggerFactory.getLogger(ExtendedParser.class);
Review Comment:
This is implicitly public. Maybe use
```
private static Logger log() {
return LoggerFactory.getLogger(ExtendedParser.class);
}
```
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488822790
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,8 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
+ logger.warn(
+ "Failed to scan delegate parser in {}: {}", parser.getClass().getName(), e.toString());
Review Comment:
done
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488799846
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -74,18 +76,15 @@ private static <T> T findParser(ParserInterface parser, Class<T> clazz) {
return clazz.cast(current);
}
- ParserInterface next = getNextDelegateParser(current);
- if (next == null) {
- break;
- }
-
- current = next;
+ current = getNextDelegateParser(current);
}
return null;
}
private static ParserInterface getNextDelegateParser(ParserInterface parser) {
+ Logger logger = LoggerFactory.getLogger(ExtendedParser.class);
Review Comment:
Interface fields cannot be private in Java.
Can the code be written like this?
```
public interface ExtendedParser extends ParserInterface {
Logger LOG = LoggerFactory.getLogger(ExtendedParser.class);
```
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488805000
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -67,6 +70,10 @@ public void saveOriginalParser() throws Exception {
try {
parserField = clazz.getDeclaredField(SQL_PARSER_FIELD);
} catch (NoSuchFieldException e) {
+ LOG.warn(
+ "Field '{}' not found in class {}. Trying superclass.",
+ SQL_PARSER_FIELD,
+ clazz.getName());
Review Comment:
I also think no need to log in tests.
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2489337881
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,7 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
+ LOG.warn("Failed to scan delegate parser in {}: {}", parser.getClass().getName(), e);
Review Comment:
without '{}' will print the exception stacktrace, which actually makes sense here.
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/ExtendedParser.java:
##########
@@ -100,6 +99,7 @@ private static ParserInterface getNextDelegateParser(ParserInterface parser) {
}
} catch (Exception e) {
// ignore
Review Comment:
done
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14497:
URL: https://github.com/apache/iceberg/pull/14497
--
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] Follow-up: Optimize support recursive delegate unwrapping to find ExtendedParser in parser chains [iceberg]
Posted by "majian1998 (via GitHub)" <gi...@apache.org>.
majian1998 commented on code in PR #14497:
URL: https://github.com/apache/iceberg/pull/14497#discussion_r2488926981
##########
spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/TestExtendedParser.java:
##########
@@ -42,7 +42,6 @@
import org.junit.jupiter.api.Test;
public class TestExtendedParser {
-
Review Comment:
Restored.
--
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