Skip to content

Render arrays correctly in SQL#3945

Closed
mamcx wants to merge 1 commit into
masterfrom
mamcx/sql-array-render
Closed

Render arrays correctly in SQL#3945
mamcx wants to merge 1 commit into
masterfrom
mamcx/sql-array-render

Conversation

@mamcx

@mamcx mamcx commented Jan 1, 2026

Copy link
Copy Markdown
Contributor

Description of Changes

As the title says.

API and ABI breaking changes

Before arrays incorrectly display and only for the first item.

Expected complexity level and risk

1

Testing

  • Smoke and unit tests
  • [] No inspected in another client outside psql
@mamcx mamcx requested a review from coolreader18 January 1, 2026 00:56
@mamcx mamcx self-assigned this Jan 1, 2026
@bfops bfops linked an issue Jan 13, 2026 that may be closed by this pull request
Comment thread crates/sats/src/ser.rs
let mut ty = &*value.ty().elem_ty;
loop {
// We're doing this because of `Ref`s.
break match (value.value(), ty) {

@egormanga egormanga Jan 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have to replicate this code in two to three places? Maybe at least add comments pointing to other places then.

But I think it can be implemented in a common trait.

Comment thread smoketests/tests/sql.py
};
ctx.db.t_others().insert(tuple.clone());
ctx.db.t_others_tuple().insert(TOthersTuple { tuple });

@egormanga egormanga Jan 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: there's an unstripped indentation left here

@egormanga

Copy link
Copy Markdown
Contributor

Not inspected in another client outside psql

Still doesn't work correctly in spacetime sql (no changes in output).

@cloutiertyler

cloutiertyler commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Needs to be reimplemented because of the out of date smoketest.

@cloutiertyler cloutiertyler added the close-stale-pr-create-issue Indicates that the PR is substantially out of date, and should become an issue to reimplement. label Apr 14, 2026
@clockwork-labs-bot

Copy link
Copy Markdown
Contributor

Closing this stale PR in favor of #4825, which tracks reimplementing the work on top of current master. Keeping this PR around as historical context and source material.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

close-stale-pr-create-issue Indicates that the PR is substantially out of date, and should become an issue to reimplement.

5 participants