Skip to content

[Enhancement] support simple sql function for factorial(from Hive)#57144

Merged
zclllyybb merged 3 commits intoapache:masterfrom
K-handle-Y:feature/factorial
Nov 10, 2025
Merged

[Enhancement] support simple sql function for factorial(from Hive)#57144
zclllyybb merged 3 commits intoapache:masterfrom
K-handle-Y:feature/factorial

Conversation

@K-handle-Y
Copy link
Contributor

@K-handle-Y K-handle-Y commented Oct 20, 2025

What problem does this PR solve?

Related PR: #48203

Problem Summary: support simple sql function for factorial(from Hive)

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

- BE: Custom IFunction in math.cpp, UT in function_math_test.cpp.
- FE: Update BuiltinScalarFunctions, signature in Factorial.java, constant folding in NumericArithmetic.java, visitor in ScalarFunctionVisitor.java.
- Tests: Regression in test_math_unary_always_nullable.groovy & fold_constant_numeric_arithmatic.groovy, updated .out; additional updates for math.cpp, NumericArithmetic.java, test_math_unary_always_nullable.groovy.

Closes apache#48203
@Thearas
Copy link
Contributor

Thearas commented Oct 20, 2025

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@zclllyybb zclllyybb self-assigned this Oct 20, 2025

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
uint32_t result, size_t input_rows_count) const override {
ColumnPtr int_col = block.get_by_position(arguments[0]).column->convert_to_full_column_if_const();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to do this, we have framework to do for const

ColumnPtr int_col = block.get_by_position(arguments[0]).column->convert_to_full_column_if_const();

const ColumnUInt8* input_null_map = nullptr;
if (int_col->is_nullable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto. we have framework for nullable input

const auto* data_col = check_and_get_column<ColumnInt64>(int_col.get());

if (!data_col) {
return Status::InvalidArgument("Illegal column {} of argument of function {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

should be InternalError for wrong type.

Copy link
Contributor

@zclllyybb zclllyybb left a comment

Choose a reason for hiding this comment

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

basically good! please also add some case:

  1. factorial(column) which column is nullable and non-nullable
  2. factorial(nullable(literal))

@K-handle-Y
Copy link
Contributor Author

basically good! please also add some case:

  1. factorial(column) which column is nullable and non-nullable
  2. factorial(nullable(literal))

done. not sure if its ok. looking forward to ur review

Copy link
Contributor

@zclllyybb zclllyybb left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 10, 2025
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@zclllyybb
Copy link
Contributor

run buildall

@doris-robot
Copy link

TPC-H: Total hot run time: 34195 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 20f4de1c88b0c07ad4872900829b325ebb9500aa, data reload: false

------ Round 1 ----------------------------------
q1	17601	5149	5018	5018
q2	2043	302	203	203
q3	10291	1364	747	747
q4	10235	943	382	382
q5	7474	2379	2331	2331
q6	186	172	135	135
q7	910	765	627	627
q8	9343	1298	1075	1075
q9	7025	5114	5191	5114
q10	6874	2239	1804	1804
q11	501	297	288	288
q12	347	374	235	235
q13	17787	3609	3100	3100
q14	235	233	220	220
q15	573	518	507	507
q16	1022	1024	942	942
q17	593	871	378	378
q18	7859	7100	6999	6999
q19	1078	963	565	565
q20	349	339	221	221
q21	3797	3212	2305	2305
q22	1096	1048	999	999
Total cold run time: 107219 ms
Total hot run time: 34195 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5110	5089	5095	5089
q2	257	329	231	231
q3	2184	2699	2299	2299
q4	1333	1811	1355	1355
q5	4174	4446	4557	4446
q6	226	178	137	137
q7	2060	2043	1860	1860
q8	2670	2597	2581	2581
q9	7441	7402	7367	7367
q10	3078	3251	2829	2829
q11	595	534	529	529
q12	667	781	610	610
q13	3591	4182	3250	3250
q14	294	306	279	279
q15	532	503	501	501
q16	1056	1185	1063	1063
q17	1183	1505	1421	1421
q18	8098	7906	7740	7740
q19	789	782	915	782
q20	1980	1999	1814	1814
q21	4785	4341	4305	4305
q22	1097	1040	1002	1002
Total cold run time: 53200 ms
Total hot run time: 51490 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 188233 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 20f4de1c88b0c07ad4872900829b325ebb9500aa, data reload: false

query1	1040	407	394	394
query2	6587	1709	1720	1709
query3	6760	229	227	227
query4	26423	23928	23233	23233
query5	4407	640	485	485
query6	371	262	242	242
query7	4649	507	308	308
query8	319	276	274	274
query9	8685	2591	2584	2584
query10	480	341	283	283
query11	15544	15098	14894	14894
query12	189	117	116	116
query13	1673	554	439	439
query14	11313	9226	9237	9226
query15	212	188	171	171
query16	7669	682	511	511
query17	1230	781	616	616
query18	2030	414	316	316
query19	217	207	177	177
query20	132	126	121	121
query21	218	133	113	113
query22	3985	4129	4075	4075
query23	34015	32963	33082	32963
query24	8475	2435	2441	2435
query25	646	519	465	465
query26	1255	270	163	163
query27	2766	497	351	351
query28	4379	2180	2175	2175
query29	815	618	496	496
query30	308	227	197	197
query31	933	828	742	742
query32	84	80	74	74
query33	591	387	342	342
query34	795	844	521	521
query35	834	842	747	747
query36	970	1004	869	869
query37	115	111	84	84
query38	3556	3574	3489	3489
query39	1471	1445	1418	1418
query40	222	132	122	122
query41	68	62	61	61
query42	134	116	116	116
query43	496	512	461	461
query44	1226	757	739	739
query45	196	186	172	172
query46	881	998	659	659
query47	1752	1765	1727	1727
query48	405	422	332	332
query49	785	564	453	453
query50	653	686	417	417
query51	3907	3925	3942	3925
query52	120	112	105	105
query53	250	279	207	207
query54	334	315	299	299
query55	91	93	89	89
query56	364	358	331	331
query57	1214	1204	1115	1115
query58	303	288	286	286
query59	2616	2680	2539	2539
query60	373	364	358	358
query61	194	193	186	186
query62	825	772	704	704
query63	233	201	204	201
query64	4627	1311	995	995
query65	4045	3959	3982	3959
query66	1122	460	362	362
query67	15386	14979	15067	14979
query68	8308	949	606	606
query69	522	333	301	301
query70	1364	1309	1206	1206
query71	498	359	334	334
query72	5933	4872	4869	4869
query73	677	582	360	360
query74	9035	9248	8715	8715
query75	4005	3335	2818	2818
query76	3675	1156	730	730
query77	820	414	318	318
query78	9575	9648	8857	8857
query79	2533	843	608	608
query80	706	573	527	527
query81	495	259	244	244
query82	427	166	134	134
query83	298	267	257	257
query84	307	113	91	91
query85	913	494	437	437
query86	338	305	311	305
query87	3751	3741	3637	3637
query88	3316	2290	2241	2241
query89	392	339	306	306
query90	2068	233	228	228
query91	181	183	141	141
query92	91	68	62	62
query93	1599	1013	645	645
query94	703	447	350	350
query95	415	326	309	309
query96	487	581	286	286
query97	2934	2999	2848	2848
query98	249	239	210	210
query99	1464	1399	1309	1309
Total cold run time: 277042 ms
Total hot run time: 188233 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 27.51 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 20f4de1c88b0c07ad4872900829b325ebb9500aa, data reload: false

query1	0.06	0.05	0.05
query2	0.09	0.05	0.05
query3	0.25	0.08	0.09
query4	1.61	0.11	0.11
query5	0.27	0.27	0.25
query6	1.17	0.64	0.65
query7	0.03	0.02	0.02
query8	0.06	0.05	0.04
query9	0.61	0.51	0.53
query10	0.58	0.58	0.58
query11	0.17	0.11	0.11
query12	0.15	0.12	0.13
query13	0.63	0.61	0.59
query14	1.00	1.00	0.99
query15	0.86	0.86	0.85
query16	0.39	0.39	0.38
query17	1.04	1.03	1.07
query18	0.22	0.20	0.20
query19	1.93	1.80	1.79
query20	0.02	0.01	0.01
query21	15.44	0.20	0.13
query22	5.05	0.07	0.05
query23	15.66	0.28	0.10
query24	3.10	1.17	0.43
query25	0.09	0.06	0.05
query26	0.14	0.14	0.14
query27	0.09	0.06	0.06
query28	4.52	1.17	0.93
query29	12.57	3.86	3.26
query30	0.28	0.14	0.11
query31	2.82	0.60	0.37
query32	3.24	0.56	0.48
query33	3.13	3.02	3.05
query34	15.86	5.17	4.58
query35	4.56	4.55	4.59
query36	0.67	0.50	0.49
query37	0.10	0.07	0.06
query38	0.06	0.04	0.04
query39	0.04	0.03	0.03
query40	0.18	0.15	0.14
query41	0.09	0.04	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.03
Total cold run time: 98.91 s
Total hot run time: 27.51 s

@hello-stephen
Copy link
Contributor

BE UT Coverage Report

Increment line coverage 87.88% (29/33) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 52.79% (18231/34538)
Line Coverage 38.14% (165803/434773)
Region Coverage 33.15% (129042/389216)
Branch Coverage 33.88% (55334/163340)

@hello-stephen
Copy link
Contributor

BE Regression && UT Coverage Report

Increment line coverage 87.88% (29/33) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 71.45% (24258/33952)
Line Coverage 57.96% (252402/435493)
Region Coverage 53.38% (210712/394737)
Branch Coverage 54.67% (89895/164423)

@hello-stephen
Copy link
Contributor

BE Regression && UT Coverage Report

Increment line coverage 87.88% (29/33) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 71.47% (24264/33952)
Line Coverage 57.97% (252468/435493)
Region Coverage 53.39% (210746/394737)
Branch Coverage 54.69% (89918/164423)

@hello-stephen
Copy link
Contributor

FE Regression Coverage Report

Increment line coverage 93.75% (15/16) 🎉
Increment coverage report
Complete coverage report

@zclllyybb zclllyybb merged commit 978cd44 into apache:master Nov 10, 2025
27 of 28 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 10, 2025
…57144)

Problem Summary: support simple sql function for factorial(from Hive)
yiguolei pushed a commit that referenced this pull request Nov 11, 2025
…rom Hive) #57144 (#57878)

Cherry-picked from #57144

Co-authored-by: K_handle_Y <2842641048@qq.com>
@K-handle-Y K-handle-Y deleted the feature/factorial branch November 11, 2025 04:16
wyxxxcat pushed a commit to wyxxxcat/doris that referenced this pull request Nov 13, 2025
…pache#57144)

Problem Summary: support simple sql function for factorial(from Hive)
wyxxxcat pushed a commit to wyxxxcat/doris that referenced this pull request Nov 18, 2025
…pache#57144)

Problem Summary: support simple sql function for factorial(from Hive)
@yiguolei yiguolei mentioned this pull request Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/4.0.2-merged reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants