Skip to content

Comments

tests: Add newline handling to slt when multiline is not specified#31930

Merged
ggevay merged 3 commits intoMaterializeInc:mainfrom
ggevay:slt-newline
Mar 19, 2025
Merged

tests: Add newline handling to slt when multiline is not specified#31930
ggevay merged 3 commits intoMaterializeInc:mainfrom
ggevay:slt-newline

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Mar 17, 2025

The first commit changes the CODEOWNERS of /src/sqllogictest from @MaterializeInc/adapter to @MaterializeInc/testing. (But let me know if there was a good reason to have the Adapter team be the owner.) Edit: Also added myself, as discussed in DMs.

The second commit fixes a minor bug in space handling in slt in colnames.

The third commit is the main thing, which adds newline handling to slt result values, even when multiline is not specified. We use the replacement character trick to avoid newlines inside result values conflicting with newlines that are for field or row separation: when rewriting, we replace newlines with ⏎; when parsing, we replace ⏎ with newlines.

The motivation is that I'd like to have this feature for my next PR, which will make SHOW CREATE ... pretty-print its output, adding newlines. We have a gazillion tests that call SHOW CREATE, and it would be painful to change all of them to use multiline, especially that they have multi-column outputs, which, I think, is not really supported with multiline.

(I've added lts-backport-v25.1, because the SHOW CREATE PR will need to be backported, because mzexplore will rely on that, and mzexplore is part of the self-managed debug tool.)

Motivation

  • This PR adds a feature that has not yet been specified.

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 requested a review from a team March 17, 2025 21:43
@ggevay ggevay added T-testing Theme: tests or test infrastructure self-managed-backport-v25.1 Needs to be backported into the v25.1 self-managed release labels Mar 17, 2025
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

The first commit changes the CODEOWNERS of /src/sqllogictest from @MaterializeInc/adapter to @MaterializeInc/testing. (But let me know if there was a good reason to have the Adapter team be the owner.)

Generally we haven't written many SLT tests in the testing team, and more focused on higher level frameworks. It still makes sense to review test changes though.

@ggevay ggevay marked this pull request as ready for review March 18, 2025 13:41
@ggevay ggevay requested a review from a team as a code owner March 18, 2025 13:41
@ggevay ggevay requested a review from ParkMyCar March 18, 2025 13:41
@ggevay ggevay mentioned this pull request Mar 18, 2025
5 tasks
Newlines are represented as a ⏎ character in the result specification.
@ggevay ggevay merged commit c2ad3d1 into MaterializeInc:main Mar 19, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

self-managed-backport-v25.1-done self-managed-backport-v25.1 Needs to be backported into the v25.1 self-managed release T-testing Theme: tests or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants