Repository navigation
fix: preserve Spark defaults in baseline runs - #3007
joeharris76 wants to merge 1 commit into
Conversation
![benchbox-oracle[bot]](/s/com/githubusercontent/avatars/G.https/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for 392b73bc24675ada759807c7b834bb573900460a
This result supersedes those for earlier heads; their open review threads still apply.
Tier: low-medium (data-only or small single-area change).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Engine-defaults direction and harness_default ledger separation are sound, but the PR also flips cache disabling, drops Spark resource defaults, and removes compression codecs outside its stated scope while explicitly reusing the same tuning-policy generation for admittedly incomparable baselines. Fix the four blocking defects before merge.
Defects without a review thread
- High
benchbox/core/tuning/policy_generation.py:7: D3: Baseline-breaking change reuses same policy generation
Spark baselines before and after this change use the same tuning_policy_generation marker adr-003 (benchbox/core/tuning/policy_generation.py:7) while docs/development/tuning-adr-003-baseline-and-single-renderer.md:111-114 admits baselines are not comparable until the owner agrees on an update. The Explorer therefore cannot warn that pre/post runs span a baseline seam, so published tuned/baseline numbers are silently incomparable and the comparability gate is weakened. Bump the generation or gate comparison.
Defects outside the diff count like the others: fix them before merge.
Reviewers not available
- luna: absent (error)
| self.staging_root = config.get("staging_root") | ||
|
|
||
| self.disable_cache = config.get("disable_cache") if config.get("disable_cache") is not None else True | ||
| self.disable_cache = config.get("disable_cache") if config.get("disable_cache") is not None else False |
There was a problem hiding this comment.
Critical: Cache-disable default flipped, enabling cache pollution
Head defaults disable_cache to False in spark.py:199, lakesail.py:97 and velox.py:143, so spark.sql.inMemoryColumnarStorage.enabled is no longer forced off and SparkDataLoadMixin/SparkQueryExecutionMixin skip catalog.clearCache (benchbox/platforms/base/spark_execution_mixin.py:363). Cached data can then leak across timed queries, making per-query times artificially fast and incomparable. The change is outside the stated AQE/CBO/shuffle/broadcast scope (CHANGELOG.md:146-149) and contradicts docs/platforms/lakesail.md:92 and docs/platforms/velox.md:97, which still document default true. Restore default True or document, gate and bump policy generation.
| self.driver_memory = config.get("driver_memory") | ||
| self.executor_memory = config.get("executor_memory") | ||
| self.executor_cores = config.get("executor_cores") |
There was a problem hiding this comment.
High: Spark resource defaults removed without versioning
Head sets driver_memory, executor_memory and executor_cores to None (no 4g/4g/2 fallback), so Spark runs on engine/cluster defaults that vary by environment instead of the previously deterministic resources. This changes memory-dependent performance and OOM behavior and is not listed in the CHANGELOG scope of AQE, CBO, shuffle partitions and broadcast thresholds (CHANGELOG.md:146-149). Either restore the resource defaults or explicitly scope, document and version the baseline change.
| @@ -373,13 +367,25 @@ def get_target_dialect(self) -> str: | |||
| def _get_spark_conf(self) -> dict[str, Any]: | |||
There was a problem hiding this comment.
Medium: Compression codec conf removed out of scope
SparkAdapter._get_spark_conf (benchbox/platforms/spark.py:367) no longer sets spark.hadoop.io.compression.codecs, previously forcing seven codecs including ZStandardCodec. Engine defaults vary by distribution/version and may lack codecs the benchmark data or CSV paths relied on, risking read failures or silent codec fallback. The removal is outside the CHANGELOG/ADR scope (AQE, CBO, shuffle, broadcast) and unversioned. Restore the codec set or prove it is harness-redundant and version the change.
Pull request was converted to draft
This branch was successfully deployed
1 participant
No description provided.