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

Add Mandelbrot set to WMR slt #31108

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Jan 18, 2025

The first commit adds a WMR test, drawing an ASCII art of the Mandelbrot set. The query is based on an SQLite example, pointed out by @umanwizard.

Had to tweak the query a bit:

  • WITH MUTUALLY RECURSIVE instead of WITH RECURSIVE
  • explicit types for each CTE
  • least instead of min (min is the aggregation function in Materialize)
  • string_agg instead of group_concat
  • chr(10) instead of x'0a'
  • added ORDER BY to the string_agg, because otherwise Materialize chooses a non-useful order inside each group.

The second commit fixes a bug in sqllogictest: When the expected output starts with a space, the trim_start() used to eat this. The trim_start was there, because the split_at(&QUERY_OUTPUT_REGEX) ends at the end of ----, leaving a newline there to be returned by the split_at that computes the output_str. Now we take care to eat just a newline instead of trim_start eating all whitespace.

(It would be tempting to just make QUERY_OUTPUT_REGEX eat the newline after ----. However, this would cause an issue for tests like joins.slt:753, where the expected output doesn't have any rows, and there is only one empty line. We expect two newlines to end the expected output, but in this test one of these newlines is the one directly after ----, so if QUERY_OUTPUT_REGEX eats this newline, then we get into trouble. One could just tweak this and other similar tests, but I wanted to make the bugfix mostly backwards compatible with existing tests. The one change that I still needed to make in an existing test was arguably in a malformed test.)

For some reason, the change in sqllogictest auto-summoned the Adapter team. But I guess it would be better if @def- reviewed the sqllogictest change. @ParkMyCar, feel free to remove yourself and the Adapter team from the reviewers.

Motivation

  • The Mandelbrot set is cool.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ggevay ggevay added the T-testing Theme: tests or test infrastructure label Jan 18, 2025
@ggevay ggevay requested a review from a team as a code owner January 18, 2025 17:12
@ggevay ggevay requested a review from ParkMyCar January 18, 2025 17:12
@ggevay ggevay force-pushed the mandelbrot branch 2 times, most recently from 9a935c7 to d91e922 Compare January 18, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-testing Theme: tests or test infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant