feat: introduce `interval_year` and `interval_day` types #48

dshaaban01 · 2025-01-01T17:16:06Z

This PR introduces two new atomic interval types:

Interval Year to Month
Supports a range of [-10,000..10,000] years, with month-level precision. Typically stored as separate integers for years and months (e.g., 12y 3m), but only the total number of months is considered significant. For example, 1y 0m is equivalent to 0y 12m or 1001y -12000m.
Interval Day to Second
Supports a range of [-3,650,000..3,650,000] days, with second precision. Typically stored as separate integers for days and seconds, but only the total number of seconds is considered significant. For example, 1d 0s is equivalent to 0d 86400s.

ingomueller-net · 2025-01-09T07:26:30Z

test/Dialect/Substrait/types.mlir

+    %0 = named_table @t1 as ["a", "b"] : tuple<!substrait.interval_year, !substrait.interval_day>
+    yield %0 : tuple<!substrait.interval_year, !substrait.interval_day>
+  }
+}


Nit: final line break.

ingomueller-net · 2025-01-21T12:43:07Z

Here as well: need to rebase again.

ingomueller-net · 2025-01-22T10:07:44Z

I think this one is next in line and needs rebasing to proceed.

dshaaban01 · 2025-02-06T14:47:51Z

Rebased and adjusted to #59. PR description also adjusted.

dshaaban01 · 2025-02-06T14:55:21Z

@ingomueller-net

I am getting this error in CI:

duckdb.duckdb.HTTPException: HTTP Error: Failed to download extension "substrait" at URL "http://extensions.duckdb.org/v1.2.0/linux_amd64_gcc4/substrait.duckdb_extension.gz" (HTTP 403)

which is causing one test to fail:

Failed Tests (1):
  Substrait MLIR :: python/dialects/substrait/e2e_duckdb.py

I guess this is an internet connection error? Any ideas on how to fix?

ingomueller-net · 2025-02-06T17:04:24Z

I ran into this this morning. This must be due to the update of DuckDB from yesterday. #79 should fix it. Rebase onto main and retry.

ingomueller-net

I looked at this PR again and found a few things we should think about a bit more.

ingomueller-net · 2025-02-07T15:48:07Z

lib/Target/SubstraitPB/Export.cpp

+    auto intervalYear =
+        mlir::cast<IntervalYearMonthAttr>(value).getYearsValue();
+    auto intervalMonth =
+        mlir::cast<IntervalYearMonthAttr>(value).getMonthsValue();


Nit: use auto only if the type appears in the RHS of a cast or similar, see LLVM Coding Standards. Also, my change factors out the cast, which makes it more readable and shorter. (Be sure to check if int32_t is the type returned by getXValue().)

Suggested change

auto intervalYear =

mlir::cast<IntervalYearMonthAttr>(value).getYearsValue();

auto intervalMonth =

mlir::cast<IntervalYearMonthAttr>(value).getMonthsValue();

auto intervalYearMonth = mlir::cast<IntervalYearMonthAttr>(value);

int32_t intervalYear = intervalYearMonth.getYearsValue();

int32_t intervalMonth = intervalYearMonth.getMonthsValue();

ingomueller-net · 2025-02-07T15:48:23Z

lib/Target/SubstraitPB/Export.cpp

+    auto intervalDay = mlir::cast<IntervalDaySecondAttr>(value).getDaysValue();
+    auto intervalSecond =
+        mlir::cast<IntervalDaySecondAttr>(value).getSecondsValue();


Same as above.

ingomueller-net · 2025-02-07T15:55:31Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

+def Substrait_IntervalYearMonthAttr 
+    : Substrait_StaticallyTypedAttr<"IntervalYearMonth", "interval_year_month",
+                                    "IntervalYearMonthType"> {
+  let summary = "Substrait interval year to month type";


I think we need to document a decision here. The spec says:

Usually stored as separate integers for years and months, but only the total number of months is significant, i.e. 1y 0m is considered equal to 0y 12m or 1001y -12000m.

We could deduce that we only want to store the number of months, then. However, I think that our design rationale suggest we should have two different numbers:

Every valid Substrait plan MUST be representable in the dialect.

I'd argue that two plans that differ by the representation of an interval_year_month are different plans (that always produce the same result), so we have to be able to express both of them with our dialect. I think that decision should be documented here.

We should, however, eventually implement a canonicalization pattern that converts these attributes to the representation with month in [0,12).

ingomueller-net · 2025-02-07T15:57:20Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

+  let description = [{
+    This type represents a substrait interval year to month attribute type.
+  }];  
+  let parameters = (ins "int32_t":$years_value, "int32_t":$months_value);


The spec only allows "a range of [-10,000..10,000] years." If we don't add a constraint now, we should add a TODO to be sure we add it later.

ingomueller-net · 2025-02-07T15:58:05Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

@@ -67,6 +67,28 @@ def Substrait_DateAttr
  let assemblyFormat = [{ `<` $value `>` }];
 }

+def Substrait_IntervalDaySecondAttr


Same two comments here as for the other interval type.

dshaaban01 · 2025-02-18T17:02:27Z

Comments addressed.

ingomueller-net · 2025-02-19T09:21:11Z

lib/Dialect/Substrait/IR/Substrait.cpp

+    int32_t month) {
+  if (year < -100000 || year > 100000)
+    return emitError() << "year must be in a range of [-10,000..10,000] years";
+  return success();


Yes, nice! month should be checked the same way (and be in [120,000..120,000]).

ingomueller-net · 2025-02-19T09:21:25Z

lib/Dialect/Substrait/IR/Substrait.cpp

+    int32_t seconds) {
+  if (days < -3650000 || days > 3650000)
+    return emitError()
+           << "days must be in a range of [-3,650,000..3,650,000] days";


Same for seconds.

ingomueller-net · 2025-02-19T09:21:51Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

+                                    "IntervalYearMonthType"> {
+  let summary = "Substrait interval year to month type";
+  let description = [{
+    This type represents a substrait interval year to month attribute type. Note that this


Please break at 80 characters.

ingomueller-net · 2025-02-19T09:23:52Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

+  let genVerifyDecl = 1;
+}
+
+def Substrait_IntervalYearMonthAttr 


Please remove this and the other trailing white spaces in the file. (Consider setting "files.trimTrailingWhitespace": true in your VS Code config.)

Suggested change

def Substrait_IntervalYearMonthAttr

def Substrait_IntervalYearMonthAttr

ingomueller-net · 2025-02-19T09:25:13Z

include/substrait-mlir/Dialect/Substrait/IR/SubstraitAttrs.td

+    This type represents a substrait interval day to second attribute type. Note that this
+    attribute does not attempt to canonicalize equivalent day-second intervals.
+  }];  
+  let parameters = (ins "int32_t":$days_value, "int32_t":$seconds_value);


I am realizing this late but: do we need the _value suffix? It's not used in the Substrait spec and I don't see any value (ha!) in having it here...

ingomueller-net · 2025-02-19T14:44:06Z

Hurray! After many rounds the PR made finally it 🎉

dshaaban01 · 2025-02-19T16:11:17Z

@ingomueller-net Did you see my comment above? We might need some more work 🫠

ingomueller-net · 2025-02-19T16:50:53Z

Which comment? The CI failure due to the DuckDB extension? That's been fixed by #79, no?

dshaaban01 · 2025-02-19T16:54:01Z

Copy/pasted comment in response to adding a constraint for seconds:

  if (seconds < -315360000000 || seconds > 315360000000)
    return emitError() << "seconds must be in a range of [-315,360,000,000.."
                          "315,360,000,000] seconds";

If I do this: 3'650'000 days * 24 hours * 60 min * 60 seconds = 315'360'000'000 seconds --> the value is larger than a int32_t max value/interval range (2,147,483,647)? Shouldn't I use an int64_t?

This is the specification that says I should use int32_t for second and (weirdly?) microsecond precision. It's also weird because there are two different example formats in the middle and right column (middle: 1d 0s and right: 3650001d -86400s 0us)? The middle column mentions microseconds but has "s" in the format, the right column mentions seconds and microseconds - I'm confused? Could there be something wrong in the spec?

ingomueller-net · 2025-02-19T17:23:38Z

Aha, I understand the problem. (I don't see the comment, though, but not a problem.)

As for the data type: the spec for the Literal submessage uses an int32, so I'd use that as well. If that means that we cannot get out of the valid range (or even cover it), then we don't have to test anything. I'd add a comment, though, or maybe even a static_assert (in the verifier, where the test would be) to be sure we think of adding a test should we ever change the type.

As for the example in the spec: the difference between the middle and the right column is that the right column has an additional 0us, right? For consistency, I guess it would be best if the middle column had that as well. But both are just informal examples, so I'd say it's rather minor. But if you want to submit a PR that fixes the example, I am pretty sure people will appreciate 😉

dshaaban01 · 2025-02-20T09:22:12Z

Okay cool got it :)) I will add a comment in a new PR and submit a PR to fix the example :)

Btw here's a screenshot of my comment, but it says "Pending"? So maybe by responding to your review comment, it assumed I was trying to review and because I don't have the rights to do so it doesn't display?

ingomueller-net · 2025-02-20T10:05:59Z

I think what happened is that you "started a review" in the right-most tab and didn't submit the review. That review mode is handy if you have a few comments for the same PR, then they get sent as one big batch. But only once you submit it. Until then, the comments are still "pending."

dshaaban01 force-pushed the dalia/types/intervals branch 3 times, most recently from 83fd62d to d6ac63f Compare January 1, 2025 17:47

ingomueller-net approved these changes Jan 9, 2025

View reviewed changes

dshaaban01 force-pushed the dalia/types/intervals branch from dd58ba1 to 296155a Compare January 9, 2025 11:51

dshaaban01 force-pushed the dalia/types/intervals branch from 82ea7b2 to 0543ca8 Compare January 16, 2025 14:16

dshaaban01 closed this Feb 6, 2025

dshaaban01 force-pushed the dalia/types/intervals branch from e9d2e3d to 131caf5 Compare February 6, 2025 14:07

dshaaban01 reopened this Feb 6, 2025

dshaaban01 mentioned this pull request Feb 6, 2025

chore: remove type of date/time attribute from test #80

Merged

dshaaban01 force-pushed the dalia/types/intervals branch 3 times, most recently from 6751b11 to 44fd3d8 Compare February 6, 2025 15:15

dshaaban01 force-pushed the dalia/types/intervals branch from 44fd3d8 to 8a6f0d8 Compare February 6, 2025 17:26

ingomueller-net reviewed Feb 7, 2025

View reviewed changes

introduce interval types

93b561a

dshaaban01 force-pushed the dalia/types/intervals branch from 8a6f0d8 to 93b561a Compare February 18, 2025 12:49

ingo comments

b1cdee4

cLaNg :clown:

f608fd4

ingomueller-net approved these changes Feb 19, 2025

View reviewed changes

dshaaban01 force-pushed the dalia/types/intervals branch 2 times, most recently from f8953cb to f608fd4 Compare February 19, 2025 12:38

ingo comments

f15c7dd

ingomueller-net merged commit b6ae72f into substrait-io:main Feb 19, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce `interval_year` and `interval_day` types #48

feat: introduce `interval_year` and `interval_day` types #48

dshaaban01 commented Jan 1, 2025 •

edited

Loading

ingomueller-net Jan 9, 2025

ingomueller-net commented Jan 21, 2025

ingomueller-net commented Jan 22, 2025

dshaaban01 commented Feb 6, 2025

dshaaban01 commented Feb 6, 2025

ingomueller-net commented Feb 6, 2025

ingomueller-net left a comment

ingomueller-net Feb 7, 2025

ingomueller-net Feb 7, 2025

ingomueller-net Feb 7, 2025

ingomueller-net Feb 7, 2025

ingomueller-net Feb 7, 2025

dshaaban01 commented Feb 18, 2025

ingomueller-net Feb 19, 2025

ingomueller-net Feb 19, 2025

ingomueller-net Feb 19, 2025

ingomueller-net Feb 19, 2025

ingomueller-net Feb 19, 2025

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 19, 2025

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 19, 2025 •

edited

Loading

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 20, 2025

ingomueller-net commented Feb 20, 2025

	def Substrait_IntervalYearMonthAttr
	def Substrait_IntervalYearMonthAttr

feat: introduce interval_year and interval_day types #48

feat: introduce interval_year and interval_day types #48

Conversation

dshaaban01 commented Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

ingomueller-net commented Jan 21, 2025

ingomueller-net commented Jan 22, 2025

dshaaban01 commented Feb 6, 2025

dshaaban01 commented Feb 6, 2025

ingomueller-net commented Feb 6, 2025

ingomueller-net left a 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

dshaaban01 commented Feb 18, 2025

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

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 19, 2025

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 19, 2025 • edited Loading

ingomueller-net commented Feb 19, 2025

dshaaban01 commented Feb 20, 2025

ingomueller-net commented Feb 20, 2025

feat: introduce `interval_year` and `interval_day` types #48

feat: introduce `interval_year` and `interval_day` types #48

dshaaban01 commented Jan 1, 2025 •

edited

Loading

dshaaban01 commented Feb 19, 2025 •

edited

Loading