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

test: add platform checks for RANGE type #18709

Conversation

nrainer-materialize
Copy link
Contributor

@nrainer-materialize nrainer-materialize commented Apr 11, 2023

Motivation

Add platform checks for RANGE types. This fixes #17815.

Status

This is a draft (see comments).

misc/python/materialize/checks/range.py Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
misc/python/materialize/checks/range.py Outdated Show resolved Hide resolved
@philip-stoev
Copy link
Contributor


[2023-04-11T08:57:25Z] rows didn't match; sleeping to see if dataflow catches up 50ms 75ms 113ms 169ms 253ms 380ms 570ms 854ms 1s 2s 3s 4s 6s 10s 15s 22s 33s 49s 74s 78s
--
  | [2023-04-11T08:57:26Z] 40:1: error: non-matching rows: expected:
  | [2023-04-11T08:57:26Z] [["U2", "A", "1000"]]
  | [2023-04-11T08:57:26Z] got:
  | [2023-04-11T08:57:26Z] [["U2", "A", "560"]]
  | [2023-04-11T08:57:26Z] Poor diff:
  | [2023-04-11T08:57:26Z] - U2 A 1000
  | [2023-04-11T08:57:26Z] + U2 A 560
  | [2023-04-11T08:57:26Z]
  | [2023-04-11T08:57:26Z]      \|
  | [2023-04-11T08:57:26Z]   39 \|
  | [2023-04-11T08:57:26Z]   40 \| > SELECT * FROM "7";
  | [2023-04-11T08:57:26Z]      \| ^


This is an unrelated regression that has been fixed on main. Consider rebasing so that you are not bothered by sporadic failures as you develop.

@nrainer-materialize nrainer-materialize marked this pull request as ready for review April 14, 2023 07:57
@nrainer-materialize nrainer-materialize force-pushed the feature/17815-range-check branch 2 times, most recently from bc49669 to 7e61da6 Compare April 14, 2023 08:51
return [
Testdrive(dedent(s))
for s in [
manipulation.replace("$view_name$", "range_view1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all good and in fact quite readable, so please do leave it as it is, but for the record the same can be achieved with format strings:

return [ Testdrive(f" range_view{i} ") for i in [1,2]]

or something along those ines.

Copy link
Contributor

@philip-stoev philip-stoev left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good and does what was needed from this test!

@nrainer-materialize
Copy link
Contributor Author

#18817 has been merged, rebased on main.

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

Successfully merging this pull request may close these issues.

platform-checks: Add RANGE type and functions that operate on it
3 participants