jinchengchenghh commented on PR #13224:
URL: https://github.com/apache/gluten/pull/13224#issuecomment-6166519496
I ask Bob to give the advice, I'm not sure if the advice is valuable, please
check if the problem exists, thanks!
Issue 1: classes HashMap in IcebergQueryTestFilter is not thread-safe
IcebergQueryTestFilter.classes is an instance-level HashMap that is mutated
during bytecode analysis (read()). The same instance METHODS is held as a
static final field in IcebergExecutionListener and called from
executionStarted() (on the JUnit TestExecutionListener thread) while the same
IcebergQueryTestFilter instance is also registered as a PostDiscoveryFilter and
invoked by the JUnit Launcher during test discovery (potentially on a different
thread). Both code paths write to classes without synchronization, which can
cause ConcurrentModificationException or silently corrupt the cache. Consider
replacing HashMap with ConcurrentHashMap, or ensuring that discovery and
execution phases never overlap on the shared instance.
Issue 2: native_proven() metric validation is vacuously true and cannot
catch zero-work executions
In Coverage.observe(), a node is added to nativeNodes only when executed is
non-empty, where executed is built by keeping only metrics whose accumulator ID
has a non-zero taskMetrics entry (taskMetrics.containsKey(value._2().id())).
This means every entry already recorded in nativeMetrics is guaranteed to have
at least one positive value. The check in summarize.py's native_proven():
all(any(value > 0 for value in entry.get("metrics", {}).values())
for entry in row.get("native_metrics", []))
is therefore always True for any non-empty native_metrics list — it can
never reject a row, making it dead validation. The real risk is a scan that
reads zero rows (e.g. an empty table in a setup-only test path): if any other
metric in executionMetrics happens to be non-zero for that operator (such as
wallNanos from merely opening a file), the node is classified as nativeNodes
and PASSED_NATIVE is reported despite no actual data being processed. The check
in native_proven() should be moved earlier — into Coverage.observe() — and
applied per-metric rather than per-entry, or a minimum threshold on at least
one data-volume metric (rawInputRows, rawInputBytes, numOutputRows,
processedSplits) should be required before a node is promoted from
unverifiedNodes to nativeNodes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
