Skip to content

Adapt rule and plugin metadata callers to Java helpers#15048

Open
gerashegalov wants to merge 2 commits into
codex/unshim-stack-02k-columnar-aggregate-statsfrom
codex/unshim-stack-02l-rule-metadata-callers
Open

Adapt rule and plugin metadata callers to Java helpers#15048
gerashegalov wants to merge 2 commits into
codex/unshim-stack-02k-columnar-aggregate-statsfrom
codex/unshim-stack-02l-rule-metadata-callers

Conversation

@gerashegalov

@gerashegalov gerashegalov commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Related to #14834.

Description

This PR is one reviewable layer in the unshim stack introduced by #15025. It updates GPU override rule and plugin metadata callers to use the moved shared Java helpers. The helper movement is already in lower layers, so this PR is limited to caller adaptation.

Stack context

Testing and validation notes

  • No standalone behavior change is intended in this layer. It is covered by the full-stack packaging/build validation described in Add default common unshim packaging flow #15025 and the existing tests for the affected subsystem.
  • The full split stack was verified to be tree-equivalent to the pre-split stack top.

Checklists

Documentation

  • Updated for new or modified user-facing features or behaviors
  • No user-facing change

Testing

  • Added or modified tests to cover new code paths
  • Covered by existing tests
    (Covered by the validation notes in the PR description.)
  • Not required

Performance

  • Tests ran and results are added in the PR description
  • Issue filed with a link in the PR description
  • Not required

@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch from ea9be01 to d6b101f Compare June 10, 2026 20:49
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02k-columnar-aggregate-stats branch from d922d5a to b94b8e4 Compare June 10, 2026 20:49
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch from d6b101f to af3b30c Compare June 10, 2026 21:13
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02k-columnar-aggregate-stats branch from b94b8e4 to 4e3e434 Compare June 10, 2026 21:13
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch from af3b30c to e1865fd Compare June 10, 2026 21:32
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02k-columnar-aggregate-stats branch from 0a315f6 to b614cf0 Compare June 10, 2026 21:36
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch 4 times, most recently from d0a0fb5 to c983119 Compare June 10, 2026 22:41
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02k-columnar-aggregate-stats branch from 413b5ee to 7a668b3 Compare June 10, 2026 22:46
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch from c983119 to 11daef2 Compare June 10, 2026 22:46
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
@gerashegalov gerashegalov force-pushed the codex/unshim-stack-02l-rule-metadata-callers branch from 11daef2 to 8a3927f Compare June 13, 2026 12:13
@gerashegalov gerashegalov marked this pull request as ready for review June 13, 2026 12:49
@greptile-apps

greptile-apps Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adapts GPU override rule and plugin metadata callers to use shared Java helpers as part of the unshim stack refactoring. The dominant pattern is converting Scala case class types (ParamCheck, RepeatingParamCheck, InputCheck, ContextChecks, ExprChecksImpl, PartChecksImpl, OomInjectionConf, and several plugin classes) to plain class … extends Serializable, replacing case class companion apply() factory calls with explicit new, and swapping extends Logging for manual slf4j loggers.

  • Shim decoupling via reflection (GpuOverrides, RapidsMeta): direct static calls to SparkShimImpl.* and AggregateInPandasExecShims.* are replaced with lazy-val-cached Class.forName / getMethod / invoke wrappers, removing compile-time dependencies on shim classes from the core module.
  • Java 9+ ByteBuffer fix (MetaUtils): packedMeta.mark() and packedMeta.reset() are cast through Buffer to avoid the covariant-return override introduced in Java 9.
  • New S3PerfReader wiring (Plugin): RapidsInputFiles.setS3PerfReader(PerfIOS3Reader.INSTANCE) is added to both the driver and executor init() paths; this bridges the sql-plugin-fileio Java interface to the Scala PerfIO$ state and activates a performance-optimized S3 read path when S3PERF_ENABLED is true.

Confidence Score: 4/5

The PR is safe to merge for existing workloads; the new S3PerfReader wiring is the only functional addition and is gated behind a config that defaults to false.

The case-class-to-class and apply-to-new mechanical transformations are correct and compile-verified by the full-stack build. The reflection-based shim dispatch is a deliberate architecture choice, not a latent bug. The open question is whether the RapidsInputFiles.setS3PerfReader calls are intentional new functionality or leaked from an adjacent layer — the PR description says no standalone behavior change, but this wiring enables the S3 performance path that was previously a no-op. The logWarning inconsistency and logger class-name loss in GpuRowBasedUserDefinedFunction are minor quality issues that do not affect correctness.

Plugin.scala — the two new setS3PerfReader call sites should be confirmed as intentional; GpuUserDefinedFunction.scala — logger bound to trait class rather than concrete class.

Important Files Changed

Filename Overview
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala Removes extends Logging; adds manual slf4j logging; replaces all ParamCheck/RepeatingParamCheck/ExprChecksImpl apply calls with new; introduces reflection-based shim dispatch helpers to decouple core from shim singletons; inlines TrampolineUtil.dataTypeExistsRecursively and confValueToString helpers.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala Converts case classes to classes with Serializable; adds manual slf4j logging; adds new RapidsInputFiles.setS3PerfReader calls in both driver and executor init — new functional behavior not described in the PR summary.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/TypeChecks.scala Converts InputCheck, ParamCheck, RepeatingParamCheck, ContextChecks, PartChecksImpl, ExprChecksImpl from case class to class extending Serializable; removes default parameter values from constructors; factory methods in ExprChecks and PartChecks companion objects preserve Scala default-argument API.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/MetaUtils.scala Adds Buffer import; casts ByteBuffer to Buffer before calling mark/reset — standard Java 9+ binary-compatibility fix. Removes extends Logging from ShuffleMetadata.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala Removes direct static import of AggregateInPandasExecShims; replaces direct shim calls with lazy-val-cached reflection lookups; replaces GpuExpressionEquals apply with new GpuExpressionEquals.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala Removes extends Logging; adds manual slf4j logging; converts OomInjectionConf and JoinOptions from case class to class with explicit toString.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuUserDefinedFunction.scala Removes with Logging from GpuRowBasedUserDefinedFunction trait; replaces with transient private lazy val log bound to classOf[GpuRowBasedUserDefinedFunction] — all subclasses now log under the trait class name rather than their concrete class name.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/shims/BloomFilterShims.scala Converts ExprChecksImpl/ContextChecks/ParamCheck apply calls to new. Adds explicit None as fourth argument to new ContextChecks now that the default value has been removed.
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/execution/GpuHashJoin.scala Replaces InputCheck apply with new InputCheck(..., Nil) consistently, providing explicit empty notes list now that the constructor default has been removed.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["GpuOverrides (object)"] -->|reflection| B["shimSingleton(name)\nClass.forName + MODULE$"]
    B --> C["invokeShimSingleton(name, method)"]
    C --> D["shimExprs / shimExecRules\nshimScanRules / shimPartRules"]
    D --> E["expressions / execs / scans / parts (val maps)"]
    A -->|lazy val| F["aggregateInPandasExecShimsModule"]
    F --> G["aggregateInPandasExecRule"]
    J["RapidsMeta (object)"] -->|lazy val| K["sparkShimImplModule\naggregateInPandasExecShimsModule"]
    K --> L["isWindowFunctionExec\nisAggregateInPandasExec"]
    M["Plugin.scala (Driver+Executor init)"] -->|NEW| N["RapidsInputFiles.setS3PerfReader(PerfIOS3Reader.INSTANCE)"]
    N --> O["sql-plugin-fileio S3PerfReader\n(enabled when S3PERF_ENABLED=true)"]
    P["case class to class conversions"] -->|Serializable| Q["ParamCheck / RepeatingParamCheck\nInputCheck / ContextChecks\nPartChecksImpl / ExprChecksImpl\nOomInjectionConf / ActiveTaskMetrics"]
Loading

Reviews (1): Last reviewed commit: "Adapt plugin metadata callers to Java he..." | Re-trigger Greptile

override def init(
sc: SparkContext, pluginContext: PluginContext): java.util.Map[String, String] = {
val sparkConf = pluginContext.conf
RapidsInputFiles.setS3PerfReader(PerfIOS3Reader.INSTANCE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 New S3PerfReader wiring appears to be unreferenced new functionality

RapidsInputFiles.setS3PerfReader(PerfIOS3Reader.INSTANCE) is called in both the driver and executor init() paths. This registers a live S3 performance reader that bridges sql-plugin's private PerfIO$ Scala object with the sql-plugin-fileio Java interface. This is not a pure caller adaptation; it introduces new runtime behavior when PerfIOConf.S3PERF_ENABLED is set to true. The PR description states "no standalone behavior change is intended in this layer." Can you confirm this S3PerfReader wiring was previously absent (i.e., the feature was already broken/no-op before this PR) and this PR intentionally enables it as part of the unshim refactor?

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants