feat: derive monotonicity for date_truc with timezone when unit larger than day #20097

st1page · 2025-01-10T11:04:24Z

…r than day

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

…r than day

src/frontend/planner_test/tests/testdata/input/temporal_filter.yaml

stdrc · 2025-01-20T07:17:57Z

src/frontend/src/optimizer/property/monotonicity.rs

+                        let date_unit = func_call.inputs()[0]
+                            .as_literal()
+                            .and_then(|literal| literal.get_data().as_ref())
+                            .map(|tz| tz.as_utf8().as_ref());
+                        if time_zone_is_without_dst(time_zone)
+                            || date_unit_larger_than_day(date_unit)
+                        {


Can you please add an example to demonstrate the correctness?

What I can think of is like in a timezone with DST, MM-dd 23:50:00 + interval 12 minutes may be MM-dd 23:02:00, while the date_trunc('day', ...) result be MM-dd 00:12:00 versus MM-dd (00:00:00). Seems this will break the correctness. Am I understanding it wrong?

it has all fields that are less significant than the selected one set to zero (or one, for day and month).
https://arc.net/l/quote/wfxyyukb

it has all fields that are less significant than the selected one set to zero (or one, for day and month).

Yes I know that.

I just got it. In my previous comment, I mixed F(X + C) with F(X) + C. I think what we need to ensure here is that, if X2 > X1, F(X2) > F(X1), and vice versa. We don't need to compare F(X + C) with F(X) + C.

related: #13201
In short:

PostgreSQL does treat units >= day and units < day differently (redotz boolean flag)

Counter-intuitively, it is possible that date_trunc('day', x) > x and the return value has hour != 0

There may be more tricky cases to think about carefully.

I think what we need to ensure here is that, if X2 > X1, F(X2) > F(X1), and vice versa.

We only have X2 > X1 implies F(X2) >= F(X1) with the equal sign

Do you mean F(X2) > F(X1) implies X2 > X1 by vice versa?

We only have X2 > X1 implies F(X2) >= F(X1) with the equal sign

Do you mean F(X2) > F(X1) implies X2 > X1 by vice versa?

Yes. Thank for the equal sign though. To be clear, I think it should be:

X2 >= X1 → F(X2) >= F(X1)

F(X2) >= F(X1) → X2 >= X1

X2 <= X1 → F(X2) <= F(X1)

F(X2) <= F(X1) → X2 <= X1

Only with the four all satisfied, we can say F maintains the monotonicity.

xxchan · 2025-01-20T07:39:01Z

src/frontend/src/optimizer/property/monotonicity.rs

+            const DAY: &str = "day";
+            const WEEK: &str = "week";
+            const MONTH: &str = "month";
+            const QUARTER: &str = "quarter";
+            const YEAR: &str = "year";
+            const DECADE: &str = "decade";
+            const CENTURY: &str = "century";
+            const MILLENNIUM: &str = "millennium";
+            date_unit.map_or(false, |date_unit| {
+                matches!(
+                    date_unit.to_ascii_lowercase().as_str(),
+                    DAY | WEEK | MONTH | QUARTER | YEAR | DECADE | CENTURY | MILLENNIUM
+                )
+            })


what's the point of the const here

In case SQL is standardized to use a language other than English in the future?

just maintain it same as the expression's implementation

risingwave/src/expr/impl/src/scalar/date_trunc.rs

Lines 20 to 33 in c06b0c6

// TODO(xiangjinwu): parse into an enum

const MICROSECONDS: &str = "microseconds";

const MILLISECONDS: &str = "milliseconds";

const SECOND: &str = "second";

const MINUTE: &str = "minute";

const HOUR: &str = "hour";

const DAY: &str = "day";

const WEEK: &str = "week";

const MONTH: &str = "month";

const QUARTER: &str = "quarter";

const YEAR: &str = "year";

const DECADE: &str = "decade";

const CENTURY: &str = "century";

const MILLENNIUM: &str = "millennium";

I mean why not use those existing definition?

It doesn't make sense to

Define const repeatedly in multiple places

Define const in-place just before its usage site

This modification is a bit troublesome; we have to expose these const from a very deep level within expr... Since we might never modify it, I would rather make a copy.

You could move these const from the deep mod into a upper layer, and import it in the original place. That should be easy to do.

I am too lazy....

Your brain will become rusty!

github-actions · 2025-04-29T02:11:51Z

This PR has been open for 60 days with no activity.

If it's blocked by code review, feel free to ping a reviewer or ask someone else to review it.

If you think it is still relevant today, and have time to work on it in the near future, you can comment to update the status, or just manually remove the no-pr-activity label.

You can also confidently close this PR to keep our backlog clean. (If no further action taken, the PR will be automatically closed after 7 days. Sorry! 🙏)
Don't worry if you think the PR is still valuable to continue in the future.
It's searchable and can be reopened when it's time. 😄

feat: derive monotonicity for date_truc with timezone when unit large…

1fed4cc

…r than day

github-actions bot added the type/feature label Jan 10, 2025

st1page changed the title ~~feat: derive monotonicity for date_truc with timezone when unit large…~~ feat: derive monotonicity for date_truc with timezone when unit larger than day Jan 10, 2025

st1page requested a review from xiangjinwu January 10, 2025 11:05

graphite-app bot reviewed Jan 10, 2025

View reviewed changes

src/frontend/planner_test/tests/testdata/input/temporal_filter.yaml Outdated Show resolved Hide resolved

typo

c06b0c6

st1page requested a review from stdrc January 15, 2025 07:42

st1page enabled auto-merge January 15, 2025 07:42

stdrc reviewed Jan 20, 2025

View reviewed changes

xxchan reviewed Jan 20, 2025

View reviewed changes

github-actions bot added the no-pr-activity label Apr 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: derive monotonicity for date_truc with timezone when unit larger than day #20097

feat: derive monotonicity for date_truc with timezone when unit larger than day #20097

st1page commented Jan 10, 2025

stdrc Jan 20, 2025

xxchan Jan 20, 2025 •

edited

Loading

stdrc Jan 20, 2025

xiangjinwu Jan 20, 2025 •

edited

Loading

stdrc Jan 20, 2025

xxchan Jan 20, 2025

BugenZhao Jan 20, 2025

st1page Jan 22, 2025

xxchan Jan 22, 2025

st1page Jan 22, 2025

xxchan Jan 22, 2025

st1page Jan 22, 2025

xxchan Jan 22, 2025

github-actions bot commented Apr 29, 2025

	// TODO(xiangjinwu): parse into an enum
	const MICROSECONDS: &str = "microseconds";
	const MILLISECONDS: &str = "milliseconds";
	const SECOND: &str = "second";
	const MINUTE: &str = "minute";
	const HOUR: &str = "hour";
	const DAY: &str = "day";
	const WEEK: &str = "week";
	const MONTH: &str = "month";
	const QUARTER: &str = "quarter";
	const YEAR: &str = "year";
	const DECADE: &str = "decade";
	const CENTURY: &str = "century";
	const MILLENNIUM: &str = "millennium";

feat: derive monotonicity for date_truc with timezone when unit larger than day #20097

Are you sure you want to change the base?

feat: derive monotonicity for date_truc with timezone when unit larger than day #20097

Conversation

st1page commented Jan 10, 2025

What's changed and what's your intention?

Checklist

Documentation

Choose a reason for hiding this comment

xxchan Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangjinwu Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2025

xxchan Jan 20, 2025 •

edited

Loading

xiangjinwu Jan 20, 2025 •

edited

Loading