fix: avoid BigDecimal.equals exception cost in FailedJoin/Table checks - #885
Open
phdoerfler wants to merge 1 commit into
Open
fix: avoid BigDecimal.equals exception cost in FailedJoin/Table checks#885phdoerfler wants to merge 1 commit into
phdoerfler wants to merge 1 commit into
Conversation
scala.math.BigDecimal.equals falls back to isValidLong (throw+catch ArithmeticException) whenever compared to a value of another type. Every FailedJoin/None sentinel check in Table.select/definesAll/ filterDefined/group/count and the SqlCursor.field assert used == against a BigDecimal-typed column value, paying full exception construction (native stack capture) on the hot row-assembly path. Async-profiler (event=cpu) showed this at 19% of total CPU time on a join-depth benchmark against AdventureWorks-for-Postgres. Replace the sentinel comparisons with reference-equality checks (FailedJoin.isFailedJoin, Table.isNone) so BigDecimal's overridden equals is never invoked for these checks. Behavior-preserving: FailedJoin and None are both singletons, so eq is exact. Kept MultiRowTable.select as an idiomatic match (not an if/else chain), using type-ascribed patterns (case v: AnyRef if v eq FailedJoin) so eq can be called directly on the scrutinee without an asInstanceOf cast.
phdoerfler
force-pushed
the
fix/bigdecimal-equals-cost
branch
from
August 2, 2026 13:09
36f738f to
882fc24
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #884.
Fun fact: This is a
scala.math.BigDecimal-specific problem, not a JVM one.java.math.BigDecimal.equalsdoesn't do this, only Scala's wrapper does.Fix
Swapped the
FailedJoin/Nonesentinel checks inTable.select/definesAll/filterDefined/group/countand theSqlCursor.fieldassert for reference-equality checks instead (FailedJoin.isFailedJoin,Table.isNone, both justv.asInstanceOf[AnyRef] eq <sentinel>). Should be exact since bothFailedJoinandNoneare singletons, and it meansBigDecimal's overriddenequalsnever gets called for these checks. Went through each changed line by hand to make sure nothing's behaving differently.An alternative fix would be to use a BigDecimal-specific type check (
case _: BigDecimal => eq check; case _ => ==). One might argue that, after all, this is just a workaround for a specific Scala limitation inBigDecimal.Or one might argue that in the end it all boils down to reference equality anyway since we are comparing sentinel values. Treating it as such also makes the check more robust. The next weird
equalsof a mapped column will no longer impact performance in this way. Also, there is a minor performance aspect to this: Making this into a BigDecimal-workaround would introduce additional type checks at runtime (and while fast, they would run often).So, I went with the universal reference equality + a comment explaining where it all started.
I also didn't limit the fix to
SqlCursor.field's assert, even though that's the one that showed up in the profiler.Table.select/definesAll/filterDefined/group/countall had the exact same pattern, all on the same row-assembly hot path, and this is shared core code used by every SQL backend (Postgres, MySQL, MSSQL, H2, ...). Scoping the fix to just the one line the profiler happened to flag would've left the rest of the bug in place.I used async-profiler to discover the issue and to verify it was gone after applying the fix. As the test data set, I used Microsoft's AdventureWorks data set. This was all found as part of a benchmark I am writing, and the data set was chosen specifically for that benchmark. To avoid relying solely on the profiler output, I also added instrumentation (I might file that as a separate PR if that seems useful), which reliably showed that the "un-flattening", or row-assembly, phase dropped by about 50%, from ~900ms down to ~450ms.