Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[isthmus] INTERVAL DAY is interpreted incorrectly #327

Open
bestbeforetoday opened this issue Feb 7, 2025 · 2 comments
Open

[isthmus] INTERVAL DAY is interpreted incorrectly #327

bestbeforetoday opened this issue Feb 7, 2025 · 2 comments

Comments

@bestbeforetoday
Copy link
Contributor

When converting an INTERVAL DAY to Substrait using Isthmus, the result interprets the interval incorrectly. For example, with this input:

select interval '1' day

The following Substrait is generated:

{
  "extensionUris": [],
  "extensions": [],
  "relations": [{
    "root": {
      "input": {
        "read": {
          "common": {
            "direct": {
            }
          },
          "baseSchema": {
            "names": ["EXPR$0"],
            "struct": {
              "types": [{
                "intervalDay": {
                  "typeVariationReference": 0,
                  "nullability": "NULLABILITY_REQUIRED",
                  "precision": 6
                }
              }],
              "typeVariationReference": 0,
              "nullability": "NULLABILITY_REQUIRED"
            }
          },
          "virtualTable": {
            "values": [{
              "fields": [{
                "intervalDayToSecond": {
                  "days": 0,
                  "seconds": 86,
                  "precision": 6,
                  "subseconds": "400000"
                },
                "nullable": false,
                "typeVariationReference": 0
              }]
            }]
          }
        }
      },
      "names": ["EXPR$0"]
    }
  }],
  "expectedTypeUrls": []
}

Note that, instead of an intervalDayToSecond with a value of 1 day, it produces a value of 86.4 seconds.

What appears to happen is that parsing the SQL produces a RexLiteral to represent the interval with the value 86400000:INTERVAL DAY. This is the number of milliseconds in a day. The conversion in the Isthmus LiteralConverter then treats the value as microseconds (using a scale of 6) and produces an incorrect result.

@bestbeforetoday
Copy link
Contributor Author

@vbarua @Blizzara I am happy to try to provide a fix but I would appreciate some guidance on exactly which part of the flow is incorrect and needs to be changed.

@Blizzara
Copy link
Contributor

Blizzara commented Feb 7, 2025

What do the "scale" and "intervalLength" values become here? if "intervalLength" is 86400000, then sounds like "scale" should be 3, and as far as I can read the logic it seems like it should work - so which part is failing?

There's also the comment // TODO: don't need to anymore - that maybe means that as the IntervalDay has a precision field, we should be able to take the given scale and use that directly (as precision - a bit confusing how decimal calls it "scale" and we call it "precision", but I guess that's for reasons). That'd still need some logic to extract the seconds and days. Or technically it could just put the whole "intervalLength" value into the subseconds field, that should also work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants