[BugFix] Support compression factor for percentile_approx #54115

INNOCENT-BOY · 2024-12-19T08:26:55Z

Why I'm doing:

I found that the runtime of compression_factor in percentile_approx hardly changes.

What I'm doing:

According to the official documentation, we can see that the percentile_approx function allows adjusting the compression. However, during actual debugging, I found that the compression remains fixed at 1000. Therefore, I implemented the content described in the function's documentation.

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

CLAassistant · 2024-12-19T08:27:01Z

All committers have signed the CLA.

be/src/exprs/agg/percentile_approx.h

be/src/util/tdigest.h

satanson · 2024-12-23T07:09:14Z

@INNOCENT-BOY add some tests in test/sql/test_agg_function/T/test_percentile_approx

kevincai · 2024-12-24T02:05:58Z

@INNOCENT-BOY clang-format check fails, please fix the code style according to the check.

INNOCENT-BOY · 2024-12-25T07:17:37Z

\

@INNOCENT-BOY clang-format check fails, please fix the code style according to the check.

I have added some test sql in tes_percentile_approx. Please check again.

INNOCENT-BOY · 2024-12-25T07:18:19Z

Hi @satanson ， please review this pr again when you are free. Thanks in advance.

stdpain · 2024-12-25T08:01:41Z

@INNOCENT-BOY
CI clang-format failed

/home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h had clang-format style issues
--- /home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h
/home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h had clang-format style issues
+++ /home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h (after clang format)
@@ -21,[9](https://github.com/StarRocks/starrocks/actions/runs/12490410567/job/34855175667?pr=54115#step:6:10) +21,7 @@
 public:
     PercentileValue() { _type = TDIGEST; }
 
-    explicit PercentileValue(double compression) : _tdigest(compression) {
-        _type = TDIGEST;
-    }
+    explicit PercentileValue(double compression) : _tdigest(compression) { _type = TDIGEST; }
 
     explicit PercentileValue(const Slice& src) {
         switch (*src.data) {

--- /home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h
+++ /home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h (after clang format)
@@ -59,7 +59,7 @@
             compression = ColumnHelper::get_const_value<TYPE_DOUBLE>(ctx->get_constant_column(2));
             if (compression < MIN_COMPRESSION || compression > MAX_COMPRESSION) {
                 LOG(WARNING) << "Compression factor out of range. Using default compression factor: "
-                << DEFAULT_COMPRESSION_FACTOR;
+                             << DEFAULT_COMPRESSION_FACTOR;
                 compression = DEFAULT_COMPRESSION_FACTOR;
             }
         }

INNOCENT-BOY · 2024-12-25T08:29:23Z

@INNOCENT-BOY CI clang-format failed

/home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h had clang-format style issues
--- /home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h
/home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h had clang-format style issues
+++ /home/runner/_work/starrocks/starrocks/be/src/util/percentile_value.h (after clang format)
@@ -21,[9](https://github.com/StarRocks/starrocks/actions/runs/12490410567/job/34855175667?pr=54115#step:6:10) +21,7 @@
 public:
     PercentileValue() { _type = TDIGEST; }
 
-    explicit PercentileValue(double compression) : _tdigest(compression) {
-        _type = TDIGEST;
-    }
+    explicit PercentileValue(double compression) : _tdigest(compression) { _type = TDIGEST; }
 
     explicit PercentileValue(const Slice& src) {
         switch (*src.data) {

--- /home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h
+++ /home/runner/_work/starrocks/starrocks/be/src/exprs/agg/percentile_approx.h (after clang format)
@@ -59,7 +59,7 @@
             compression = ColumnHelper::get_const_value<TYPE_DOUBLE>(ctx->get_constant_column(2));
             if (compression < MIN_COMPRESSION || compression > MAX_COMPRESSION) {
                 LOG(WARNING) << "Compression factor out of range. Using default compression factor: "
-                << DEFAULT_COMPRESSION_FACTOR;
+                             << DEFAULT_COMPRESSION_FACTOR;
                 compression = DEFAULT_COMPRESSION_FACTOR;
             }
         }

Hi @stdpain , I have fixed format issue, please check again.

Seaven · 2024-12-25T12:05:05Z

main RELEASE (build 5a005563a0)
query_id:00000000-0000-0000-0000-000000000000, fragment_instance:00000000-0000-0000-0000-000000000000
tracker:process consumption: 612340656
tracker:jemalloc_metadata consumption: 34296544
tracker:query_pool consumption: 11065688
tracker:query_pool/connector_scan consumption: 0
tracker:load consumption: 20928
tracker:metadata consumption: 6475282
tracker:tablet_metadata consumption: 2179810
tracker:rowset_metadata consumption: 1765131
tracker:segment_metadata consumption: 461493
tracker:column_metadata consumption: 2068848
tracker:tablet_schema consumption: 169610
tracker:segment_zonemap consumption: 300278
tracker:short_key_index consumption: 15053
tracker:column_zonemap_index consumption: 534336
tracker:ordinal_index consumption: 386944
tracker:bitmap_index consumption: 0
tracker:bloom_filter_index consumption: 0
tracker:compaction consumption: 0
tracker:schema_change consumption: 0
tracker:page_cache consumption: 1687856
tracker:jit_cache consumption: 60480
tracker:update consumption: 244859
tracker:chunk_allocator consumption: 0
tracker:passthrough consumption: 0
tracker:clone consumption: 0
tracker:consistency consumption: 0
tracker:datacache consumption: 0
tracker:poco_connection_pool consumption: 769208
tracker:replication consumption: 0
*** Aborted at 1735121553 (unix time) try "date -d @1735121553" if you are using GNU date ***
PC: @          0x6e6ca56 starrocks::PercentileApproxAggregateFunction::create(starrocks::FunctionContext*, unsigned char*) const
*** SIGSEGV (@0x10) received by PID 7303 (TID 0x7f0c504b0640) LWP(41142) from PID 16; stack trace: ***
    @     0x7f0d9efe4ee8 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x99ee7)
    @          0xfaccc59 google::(anonymous namespace)::FailureSignalHandler(int, siginfo_t*, void*)
    @     0x7f0d9fec0486 os::Linux::chained_handler(int, siginfo_t*, void*)
    @     0x7f0d9fec678b JVM_handle_linux_signal
    @     0x7f0d9feb878c signalHandler(int, siginfo_t*, void*)
    @     0x7f0d9ef8d520 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4251f)
    @          0x6e6ca56 starrocks::PercentileApproxAggregateFunction::create(starrocks::FunctionContext*, unsigned char*) const
    @          0x6e45941 starrocks::AggStateUnion::create(starrocks::FunctionContext*, unsigned char*) const
    @          0x6e4e738 starrocks::AggFuncBasedValueAggregator::AggFuncBasedValueAggregator(starrocks::AggStateDesc*, std::unique_ptr<starrocks::AggregateFunction, std::default_delete<starrocks::AggregateFunction> >)
    @          0x6e42aac starrocks::ColumnAggregatorFactory::create_value_column_aggregator(std::shared_ptr<starrocks::Field> const&)
    @          0x6e3eaee starrocks::ChunkAggregator::ChunkAggregator(starrocks::Schema const*, unsigned int, unsigned int, double, bool, bool)
    @          0x6e3ec6c starrocks::ChunkAggregator::ChunkAggregator(starrocks::Schema const*, unsigned int, unsigned int, double)
    @          0x8c6de0a starrocks::MemTable::_init_aggregator_if_needed()
    @          0x8c6e32d starrocks::MemTable::MemTable(long, starrocks::Schema const*, std::vector<starrocks::SlotDescriptor*, std::allocator<starrocks::SlotDescriptor*> > const*, starrocks::MemTableSink*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<cha��
    @          0x8d46451 starrocks::DeltaWriter::_reset_mem_table()
    @          0x8d49279 starrocks::DeltaWriter::write(starrocks::Chunk const&, unsigned int const*, unsigned int, unsigned int)
    @          0xb00caf4 starrocks::AsyncDeltaWriter::_execute(void*, bthread::TaskIterator<starrocks::AsyncDeltaWriter::Task>&)
    @          0xb00f839 bthread::ExecutionQueue<starrocks::AsyncDeltaWriter::Task>::execute_task(void*, void*, bthread::TaskIteratorBase&)
    @          0xfccde8f bthread::ExecutionQueueBase::_execute(bthread::TaskNode*, bool, int*)
    @          0xfccefbb bthread::ExecutionQueueBase::_execute_tasks(void*)
    @          0xb2b75ea std::_Function_handler<void (), starrocks::bthreads::ThreadPoolExecutor::submit(void* (*)(void*), void*)::{lambda()#1}>::_M_invoke(std::_Any_data const&)
    @          0xb41e238 starrocks::ThreadPool::dispatch_thread()
    @          0xb41f4f5 std::_Function_handler<void (), std::_Bind<void (starrocks::ThreadPool::*(starrocks::ThreadPool*))()> >::_M_invoke(std::_Any_data const&)
    @          0xb41340b starrocks::Thread::supervise_thread(void*)
    @     0x7f0d9efdfac3 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
    @     0x7f0d9f071850 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x12684f)

@LiShuMing be crash in MV cases, help plz

LiShuMing · 2024-12-27T05:54:10Z

be/src/exprs/agg/percentile_approx.h

 public:
+    void create(FunctionContext* ctx, AggDataPtr __restrict ptr) const override {


Can you give a default compression if ctx is null because agg_state in common aggregate state it may be null.

INNOCENT-BOY · 2024-12-27T09:32:16Z

Hi @kevincai @LiShuMing @stdpain , please help review this pr again.

stdpain · 2024-12-27T11:14:46Z

@INNOCENT-BOY check the test case: test_agg_state_table_with_all_functions

LiShuMing · 2024-12-31T03:31:34Z

@mergify rebase

mergify · 2024-12-31T03:31:57Z

rebase

✅ Branch has been successfully rebased

LiShuMing · 2024-12-31T08:09:37Z

I found a bug in original sql test, you can rebase the code after pr(#54556) is merged.

LiShuMing · 2024-12-31T10:32:32Z

@mergify rebase

mergify · 2024-12-31T10:32:56Z

rebase

✅ Branch has been successfully rebased

INNOCENT-BOY · 2025-01-07T05:27:30Z

Hi @LiShuMing @satanson ,What do I need to do to get this PR merged?

github-actions · 2025-01-07T10:22:57Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2025-01-07T10:23:07Z

[FE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2025-01-07T10:34:09Z

[BE Incremental Coverage Report]

✅ pass : 13 / 16 (81.25%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/exprs/agg/percentile_approx.h	12	15	80.00%	[58, 59, 60]
🔵	be/src/util/percentile_value.h	1	1	100.00%	[]

INNOCENT-BOY requested review from a team as code owners December 19, 2024 08:26

github-actions bot added behavior_changed title needs [type] labels Dec 19, 2024

mergify bot assigned INNOCENT-BOY Dec 19, 2024

INNOCENT-BOY changed the title ~~support compression factor for percentile_approx~~ [BugFix] Support compression factor for percentile_approx Dec 19, 2024

github-actions bot removed the title needs [type] label Dec 19, 2024

starrocks-cr bot reviewed Dec 19, 2024

View reviewed changes

be/src/exprs/agg/percentile_approx.h Show resolved Hide resolved

github-actions bot removed the behavior_changed label Dec 19, 2024

INNOCENT-BOY force-pushed the main branch from c85d003 to 7eeca3a Compare December 19, 2024 09:42

INNOCENT-BOY removed their assignment Dec 19, 2024

mergify bot assigned INNOCENT-BOY Dec 19, 2024

satanson reviewed Dec 20, 2024

View reviewed changes

be/src/util/tdigest.h Outdated Show resolved Hide resolved

INNOCENT-BOY force-pushed the main branch from 7eeca3a to 692a89e Compare December 20, 2024 10:04

INNOCENT-BOY requested a review from satanson December 20, 2024 10:04

stdpain previously approved these changes Dec 23, 2024

View reviewed changes

INNOCENT-BOY dismissed stdpain’s stale review via a54443b December 25, 2024 06:48

INNOCENT-BOY force-pushed the main branch from 692a89e to a54443b Compare December 25, 2024 06:48

INNOCENT-BOY requested a review from stdpain December 25, 2024 06:50

INNOCENT-BOY force-pushed the main branch from a54443b to 5a00556 Compare December 25, 2024 08:27

LiShuMing previously approved these changes Dec 27, 2024

View reviewed changes

INNOCENT-BOY dismissed LiShuMing’s stale review via f2f3d87 December 27, 2024 07:19

INNOCENT-BOY force-pushed the main branch from 5a00556 to f2f3d87 Compare December 27, 2024 07:19

INNOCENT-BOY requested a review from LiShuMing December 27, 2024 07:49

satanson approved these changes Dec 30, 2024

View reviewed changes

LiShuMing approved these changes Dec 31, 2024

View reviewed changes

LiShuMing force-pushed the main branch from f2f3d87 to 291b424 Compare December 31, 2024 03:31

support compression factor for percentile_approx

b526abf

LiShuMing force-pushed the main branch from 291b424 to b526abf Compare December 31, 2024 10:32

stdpain approved these changes Jan 7, 2025

View reviewed changes

Seaven enabled auto-merge (squash) January 7, 2025 12:45

dirtysalt approved these changes Jan 7, 2025

View reviewed changes

Seaven merged commit 2b225a7 into StarRocks:main Jan 7, 2025
43 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] Support compression factor for percentile_approx #54115

[BugFix] Support compression factor for percentile_approx #54115

INNOCENT-BOY commented Dec 19, 2024 •

edited by satanson

Loading

CLAassistant commented Dec 19, 2024 •

edited

Loading

satanson commented Dec 23, 2024

kevincai commented Dec 24, 2024

INNOCENT-BOY commented Dec 25, 2024

INNOCENT-BOY commented Dec 25, 2024

stdpain commented Dec 25, 2024 •

edited

Loading

INNOCENT-BOY commented Dec 25, 2024

Seaven commented Dec 25, 2024 •

edited

Loading

LiShuMing Dec 27, 2024

INNOCENT-BOY Dec 27, 2024

INNOCENT-BOY commented Dec 27, 2024

stdpain commented Dec 27, 2024

LiShuMing commented Dec 31, 2024

mergify bot commented Dec 31, 2024

LiShuMing commented Dec 31, 2024

LiShuMing commented Dec 31, 2024

mergify bot commented Dec 31, 2024

INNOCENT-BOY commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

		public:
		void create(FunctionContext* ctx, AggDataPtr __restrict ptr) const override {

[BugFix] Support compression factor for percentile_approx #54115

[BugFix] Support compression factor for percentile_approx #54115

Conversation

INNOCENT-BOY commented Dec 19, 2024 • edited by satanson Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

CLAassistant commented Dec 19, 2024 • edited Loading

satanson commented Dec 23, 2024

kevincai commented Dec 24, 2024

INNOCENT-BOY commented Dec 25, 2024

INNOCENT-BOY commented Dec 25, 2024

stdpain commented Dec 25, 2024 • edited Loading

INNOCENT-BOY commented Dec 25, 2024

Seaven commented Dec 25, 2024 • edited Loading

LiShuMing Dec 27, 2024

Choose a reason for hiding this comment

INNOCENT-BOY Dec 27, 2024

Choose a reason for hiding this comment

INNOCENT-BOY commented Dec 27, 2024

stdpain commented Dec 27, 2024

LiShuMing commented Dec 31, 2024

mergify bot commented Dec 31, 2024

✅ Branch has been successfully rebased

LiShuMing commented Dec 31, 2024

LiShuMing commented Dec 31, 2024

mergify bot commented Dec 31, 2024

✅ Branch has been successfully rebased

INNOCENT-BOY commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Jan 7, 2025

[FE Incremental Coverage Report]

github-actions bot commented Jan 7, 2025

[BE Incremental Coverage Report]

file detail

INNOCENT-BOY commented Dec 19, 2024 •

edited by satanson

Loading

CLAassistant commented Dec 19, 2024 •

edited

Loading

stdpain commented Dec 25, 2024 •

edited

Loading

Seaven commented Dec 25, 2024 •

edited

Loading