Skip to content

fix: avoid BigDecimal.equals exception cost in FailedJoin/Table checks - #885

Merged
milessabin merged 1 commit into
typelevel:mainfrom
phdoerfler:fix/bigdecimal-equals-cost
Aug 6, 2026
Merged

fix: avoid BigDecimal.equals exception cost in FailedJoin/Table checks#885
milessabin merged 1 commit into
typelevel:mainfrom
phdoerfler:fix/bigdecimal-equals-cost

Conversation

@phdoerfler

@phdoerfler phdoerfler commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #884.

Fun fact: This is a scala.math.BigDecimal-specific problem, not a JVM one. java.math.BigDecimal.equals doesn't do this, only Scala's wrapper does.

Fix

Swapped the FailedJoin/None sentinel checks in Table.select/definesAll/filterDefined/group/count and the SqlCursor.field assert for reference-equality checks instead (FailedJoin.isFailedJoin, Table.isNone, both just v.asInstanceOf[AnyRef] eq <sentinel>). Should be exact since both FailedJoin and None are singletons, and it means BigDecimal's overridden equals never 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 in BigDecimal.
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 equals of 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/count all 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.

@phdoerfler
phdoerfler force-pushed the fix/bigdecimal-equals-cost branch from 36f738f to 882fc24 Compare August 2, 2026 13:09
@phdoerfler

phdoerfler commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

I have pushed an update with a simpler fix. All that is actually needed is to swap the sides of .equals to avoid the costly BigDecimal.equalsisFailedJoin/isNone now just keep the sentinel on the left of == instead of using asInstanceOf/eq. Same effect, smaller diff, call sites unchanged.

The bare case FailedJoin => / case None => arms are back to their original form too — never needed the eq-guard.

@phdoerfler
phdoerfler force-pushed the fix/bigdecimal-equals-cost branch from 3e887c8 to b19dfaa Compare August 3, 2026 12:48
@phdoerfler
phdoerfler force-pushed the fix/bigdecimal-equals-cost branch from 5523acd to ed9f25d Compare August 3, 2026 15:25

@milessabin milessabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@milessabin
milessabin merged commit 1090d18 into typelevel:main Aug 6, 2026
13 checks passed
@phdoerfler
phdoerfler deleted the fix/bigdecimal-equals-cost branch August 8, 2026 13:50
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.

Expensive calls to BigDecimal.equals

2 participants