Skip to content

Comments

Revert "fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers (#14223)"#14292

Closed
alamb wants to merge 1 commit intoapache:mainfrom
alamb:alamb/revert_14291
Closed

Revert "fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers (#14223)"#14292
alamb wants to merge 1 commit intoapache:mainfrom
alamb:alamb/revert_14291

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jan 25, 2025 •
edited
Loading

Draft PR to test the performance implications of #14223.

Which issue does this PR close?

Rationale for this change

There is concern that #14223 will slow down performance

I would like to gather some real data to find out, so I am preparing a branch to test. I don't know yet if I will propose to merge this

Update, my measurements suggest there is no change in benchmark performance

What changes are included in this PR?

Reverts

Are these changes tested?

I ran benchmark results;

Clickbench

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃  main_base ┃ alamb_revert_14291 ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.33ms │             2.30ms │     no change │
│ QQuery 1     │    40.38ms │            39.35ms │     no change │
│ QQuery 2     │    98.00ms │            99.64ms │     no change │
│ QQuery 3     │   108.62ms │           105.25ms │     no change │
│ QQuery 4     │   923.58ms │           934.56ms │     no change │
│ QQuery 5     │   966.92ms │           956.66ms │     no change │
│ QQuery 6     │    38.23ms │            38.43ms │     no change │
│ QQuery 7     │    44.28ms │            42.90ms │     no change │
│ QQuery 8     │  1053.30ms │          1022.30ms │     no change │
│ QQuery 9     │  1343.71ms │          1351.18ms │     no change │
│ QQuery 10    │   295.52ms │           281.84ms │     no change │
│ QQuery 11    │   340.04ms │           324.68ms │     no change │
│ QQuery 12    │  1007.39ms │           967.09ms │     no change │
│ QQuery 13    │  1296.34ms │          1292.14ms │     no change │
│ QQuery 14    │   902.32ms │           905.02ms │     no change │
│ QQuery 15    │  1086.61ms │          1087.18ms │     no change │
│ QQuery 16    │  2016.05ms │          1971.24ms │     no change │
│ QQuery 17    │  1852.81ms │          1847.69ms │     no change │
│ QQuery 18    │  3995.11ms │          3995.56ms │     no change │
│ QQuery 19    │    97.99ms │            96.54ms │     no change │
│ QQuery 20    │  1258.22ms │          1249.90ms │     no change │
│ QQuery 21    │  1495.27ms │          1502.42ms │     no change │
│ QQuery 22    │  2714.73ms │          2649.74ms │     no change │
│ QQuery 23    │  8924.91ms │          8700.99ms │     no change │
│ QQuery 24    │   528.87ms │           498.62ms │ +1.06x faster │
│ QQuery 25    │   434.64ms │           426.47ms │     no change │
│ QQuery 26    │   597.96ms │           578.93ms │     no change │
│ QQuery 27    │  1841.25ms │          1805.83ms │     no change │
│ QQuery 28    │ 13032.24ms │         12870.17ms │     no change │
│ QQuery 29    │   530.51ms │           538.89ms │     no change │
│ QQuery 30    │   915.49ms │           905.28ms │     no change │
│ QQuery 31    │   966.31ms │           951.39ms │     no change │
│ QQuery 32    │  3739.78ms │          3984.97ms │  1.07x slower │
│ QQuery 33    │  3999.29ms │          4091.47ms │     no change │
│ QQuery 34    │  4045.53ms │          4093.23ms │     no change │
│ QQuery 35    │  1369.10ms │          1365.20ms │     no change │
│ QQuery 36    │   238.67ms │           232.64ms │     no change │
│ QQuery 37    │    98.54ms │            97.98ms │     no change │
│ QQuery 38    │   137.36ms │           138.82ms │     no change │
│ QQuery 39    │   450.38ms │           435.35ms │     no change │
│ QQuery 40    │    56.76ms │            55.85ms │     no change │
│ QQuery 41    │    49.64ms │            48.00ms │     no change │
│ QQuery 42    │    64.35ms │            64.80ms │     no change │
└──────────────┴────────────┴────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary                 ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (main_base)            │ 64999.31ms │
│ Total Time (alamb_revert_14291)   │ 64648.51ms │
│ Average Time (main_base)          │  1511.61ms │
│ Average Time (alamb_revert_14291) │  1503.45ms │
│ Queries Faster                    │          1 │
│ Queries Slower                    │          1 │
│ Queries with No Change            │         41 │
└───────────────────────────────────┴────────────┘

TPCH

--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃ main_base ┃ alamb_revert_14291 ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1     │  129.31ms │           131.15ms │     no change │
│ QQuery 2     │   26.78ms │            27.34ms │     no change │
│ QQuery 3     │   40.49ms │            41.67ms │     no change │
│ QQuery 4     │   22.81ms │            22.92ms │     no change │
│ QQuery 5     │   64.09ms │            65.19ms │     no change │
│ QQuery 6     │    8.79ms │             8.59ms │     no change │
│ QQuery 7     │  129.23ms │           125.27ms │     no change │
│ QQuery 8     │   28.69ms │            29.49ms │     no change │
│ QQuery 9     │   75.24ms │            65.05ms │ +1.16x faster │
│ QQuery 10    │   60.30ms │            60.58ms │     no change │
│ QQuery 11    │   13.28ms │            13.39ms │     no change │
│ QQuery 12    │   39.85ms │            38.07ms │     no change │
│ QQuery 13    │   29.33ms │            29.23ms │     no change │
│ QQuery 14    │   10.49ms │            11.72ms │  1.12x slower │
│ QQuery 15    │   27.89ms │            28.80ms │     no change │
│ QQuery 16    │   23.40ms │            23.00ms │     no change │
│ QQuery 17    │  105.40ms │           100.46ms │     no change │
│ QQuery 18    │  247.95ms │           257.37ms │     no change │
│ QQuery 19    │   30.43ms │            30.58ms │     no change │
│ QQuery 20    │   42.96ms │            41.67ms │     no change │
│ QQuery 21    │  171.44ms │           180.23ms │  1.05x slower │
│ QQuery 22    │   18.16ms │            17.59ms │     no change │
└──────────────┴───────────┴────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary                 ┃           ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (main_base)            │ 1346.32ms │
│ Total Time (alamb_revert_14291)   │ 1349.33ms │
│ Average Time (main_base)          │   61.20ms │
│ Average Time (alamb_revert_14291) │   61.33ms │
│ Queries Faster                    │         1 │
│ Queries Slower                    │         2 │
│ Queries with No Change            │        19 │
└───────────────────────────────────┴───────────┘

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 25, 2025
This was referenced Jan 25, 2025
Merged
Open
@alamb
Copy link
Contributor Author

alamb commented Feb 22, 2025

I think we decided we don't need to do this

@alamb alamb closed this Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@alamb