Skip to content

support array_to_json and row_to_json functions#2730

Merged
jennifersp merged 4 commits into
mainfrom
jennifer/fixes
May 21, 2026
Merged

support array_to_json and row_to_json functions#2730
jennifersp merged 4 commits into
mainfrom
jennifer/fixes

Conversation

@jennifersp
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Main PR
covering_index_scan_postgres 1325.86/s 1340.82/s +1.1%
index_join_postgres 190.41/s 191.03/s +0.3%
index_join_scan_postgres 199.66/s 199.61/s -0.1%
index_scan_postgres 12.53/s 12.59/s +0.4%
oltp_point_select 2442.95/s 2458.03/s +0.6%
oltp_read_only 1823.61/s 1845.61/s +1.2%
select_random_points 130.33/s 131.95/s +1.2%
select_random_ranges 876.22/s 890.90/s +1.6%
table_scan_postgres 12.23/s 12.26/s +0.2%
types_table_scan_postgres 5.58/s 5.63/s +0.8%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

Main PR
Total 42090 42090
Successful 18126 18150
Failures 23964 23940
Partial Successes1 5384 5392
Main PR
Successful 43.0649% 43.1219%
Failures 56.9351% 56.8781%

${\color{lightgreen}Progressions (25)}$

create_view

QUERY: CREATE VIEW mysecview3 WITH (security_barrier=false)
       AS SELECT * FROM tbl1 WHERE a < 0;
QUERY: CREATE VIEW mysecview4 WITH (security_barrier)
       AS SELECT * FROM tbl1 WHERE a <> 0;
QUERY: CREATE OR REPLACE VIEW mysecview4 WITH (security_barrier=false)
       AS SELECT * FROM tbl1 WHERE a <> 256;

json

QUERY: SELECT array_to_json(array(select 1 as a));
QUERY: SELECT row_to_json(row(1,'foo'));

jsonb

QUERY: SELECT array_to_json(ARRAY [jsonb '{"a":1}', jsonb '{"b":[2,3]}']);

rowsecurity

QUERY: CREATE VIEW bv1 WITH (security_barrier) AS SELECT * FROM b1 WHERE a > 0 WITH CHECK OPTION;

rowtypes

QUERY: select row(1, 2)::testtype1 < row(1, 3)::testtype1;
QUERY: select row(1, 2)::testtype1 <= row(1, 3)::testtype1;
QUERY: select row(1, 2)::testtype1 = row(1, 2)::testtype1;
QUERY: select row(1, 2)::testtype1 <> row(1, 3)::testtype1;
QUERY: select row(1, 3)::testtype1 >= row(1, 2)::testtype1;
QUERY: select row(1, 3)::testtype1 > row(1, 2)::testtype1;
QUERY: select row(1, -2)::testtype1 = row(1, -3)::testtype1;
QUERY: select row(1, -3)::testtype1 >= row(1, -2)::testtype1;
QUERY: select row(1, -2)::testtype1 < row(1, 3)::testtype1;
QUERY: select row_to_json(i) from int8_tbl i;
QUERY: select row_to_json(i) from int8_tbl i(x,y);

select_parallel

QUERY: CREATE VIEW tenk1_vw_sec WITH (security_barrier) AS SELECT * FROM tenk1;

select_views

QUERY: CREATE VIEW my_property_secure WITH (security_barrier) AS
       SELECT * FROM customer WHERE name = current_user;
QUERY: CREATE VIEW my_credit_card_secure WITH (security_barrier) AS
       SELECT * FROM customer l NATURAL JOIN credit_card r
       WHERE l.name = current_user;
QUERY: CREATE VIEW my_credit_card_usage_normal AS
       SELECT * FROM my_credit_card_secure l NATURAL JOIN credit_usage r;
QUERY: CREATE VIEW my_credit_card_usage_secure WITH (security_barrier) AS
       SELECT * FROM my_credit_card_secure l NATURAL JOIN credit_usage r;

updatable_views

QUERY: create view wcowrtest_v2 as
    select *
      from wcowrtest r
      where r in (select s from sometable s where r.a = s.a)
with check option;
QUERY: drop view wcowrtest_v, wcowrtest_v2;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@itoqa
Copy link
Copy Markdown

itoqa Bot commented May 19, 2026

Ito Test Report ❌

10 test cases ran. 1 failed, 9 passed.

Across the unified run, 10 test cases were recorded with 9 passes and 1 failure (plus one execution that ended before any verifiable test outcome), indicating generally stable behavior with a single confirmed defect. The key finding was a medium, PR-introduced bug where row_to_json(d.*) on a derived-table alias fails with a composite-type creation error, while other checks confirmed expected behavior: table-backed row_to_json preserves named keys, anonymous/projection forms intentionally fall back to f1/f2/f3, and CREATE VIEW security_barrier handling is intentionally split (false accepted, true rejected) with policy enforcement remaining consistent.

❌ Failed (1)
Category Summary Screenshot
Resolve 🟠 row_to_json(d.*) fails on a derived-table alias with composite-type creation error instead of returning schema-keyed JSON. RESOLVE-2
🟠 Derived-table alias composite resolution fails
  • What failed: The derived-table alias invocation errors with could not create a composite type for table "d"; expected behavior is consistent record resolution and JSON rendering for equivalent row sources.
  • Impact: A meaningful row_to_json invocation path breaks for derived-table aliases, causing query failures in SQL workflows that rely on aliased subqueries. Users can sometimes rewrite queries to avoid d.*, but behavior is inconsistent across equivalent forms.
  • Steps to reproduce:
    1. Create rtj_resolve2(user_id, display_name, active_flag) and insert a row.
    2. Run SELECT row_to_json(t), row_to_json(t.*) FROM rtj_resolve2 t and confirm both invocations return named-key JSON.
    3. Run SELECT row_to_json(d.*) FROM (SELECT * FROM rtj_resolve2) d and observe the composite-type creation error for alias d.
  • Stub / mock context: Authorization-role checks were temporarily bypassed for regression execution so SQL function behavior could be validated in a deterministic local runtime. No stubs or mocked query responses were used for the row_to_json derived-table query itself.
  • Code analysis: I inspected the composite conversion and function-resolution path. Record/composite compatibility is intentionally allowed, and JSON rendering correctly uses composite attribute names when type metadata exists; however, the table-to-composite builder looks up alias d as a concrete type name with empty schema and hard-fails when no matching composite type is found.
  • Why this is likely a bug: Equivalent composite-as-record invocation paths should not diverge based only on derived-table alias naming, and the hard failure is directly explained by alias-based type lookup logic in production code.

Relevant code:

server/expression/table_to_composite.go (lines 42-49)

// TODO: we need to get the schema, but the GMS builder doesn't have that information
typ, err := coll.GetType(ctx, id.NewType("", tableName))
if err != nil {
	return nil, err
}
if typ == nil {
	return nil, errors.New(fmt.Sprintf(`could not create a composite type for table "%s"`, tableName))
}

server/functions/framework/compiled_function.go (lines 618-626)

} else if paramType.IsRecordType() && argTypes[i].IsCompositeType() {
	// Composite types (e.g. table row types) are compatible with the generic Record parameter.
	overloadCasts[i] = casts.Cast{
		ID:       id.NewCast(argTypes[i].ID, paramType.ID),
		CastType: casts.CastType_Implicit,
		Function: id.NullFunction,
		UseInOut: false,
	}

server/functions/row_to_json.go (lines 98-104)

func fieldNames(rowType *pgtypes.DoltgresType, count int) []string {
	names := make([]string, count)
	if rowType != nil && len(rowType.CompositeAttrs) == count {
		for i, attr := range rowType.CompositeAttrs {
			names[i] = attr.Name
		}
	} else {
✅ Passed (9)
Category Summary Screenshot
Render Table-backed row_to_json returned {"id":1,"name":"Alice"}, preserving composite attribute names. N/A
Render Anonymous row conversion returned compact JSON with synthetic keys f1/f2/f3 and correct scalar/boolean/null semantics. N/A
Render Code-first verification confirmed fallback to f1/f2 for anonymous projection shapes is intentional behavior, not a defect. N/A
Resolve Composite table rows successfully resolve to row_to_json(record) and return named JSON keys. RESOLVE-1
View CREATE VIEW ... WITH (security_barrier=false) succeeded and the resulting view returned one row with c=1 via the application SQL path. N/A
View CREATE VIEW with security_barrier=true was rejected with an unsupported-option error and no view object was created. N/A
View Under the same restricted-role validation path, CREATE VIEW statements with and without security_barrier=false both failed with permission denied, indicating parser acceptance did not bypass policy enforcement. N/A
View Re-test confirms deterministic, explicit behavior in source and runtime: CREATE VIEW ... security_barrier=true is intentionally rejected while equivalent security_barrier=false is intentionally accepted and queryable. This matches implementation logic rather than an infrastructure/tooling failure or hidden parser defect. N/A
View Replayed migration-style statements show explicit behavioral split (true rejected, false accepted). This is a deliberate parser behavior change present in source, not a flaky execution artifact. No hidden runtime inconsistency was observed in the application path tested. N/A

Commit: c958b34

View Full Run


Tell us how we did: Give Ito Feedback

@jennifersp jennifersp requested a review from Hydrocharged May 19, 2026 18:05
Copy link
Copy Markdown
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread server/functions/array_to_json.go Outdated
Comment thread server/functions/framework/compiled_function.go Outdated
@jennifersp jennifersp enabled auto-merge (squash) May 21, 2026 17:12
@itoqa
Copy link
Copy Markdown

itoqa Bot commented May 21, 2026

Ito Diff Report ❌

Tested: 5f7ead2744ba6a
7 test cases ran this commit: 3 passed ✅, 3 failed ❌, 1 additional finding ⚠️.
↪️ Carried forward from prior run (not retested this commit): 12 passing.

Across 7 test cases, 3 passed and 4 failed, confirming multiple real product defects despite some successful baseline behavior. The most critical findings were a High-severity import blocker where dumps fail on unsupported ALTER DEFAULT PRIVILEGES statements and a High-severity row_to_json inconsistency for derived composite rows/casts, alongside Medium-severity issues where CREATE VIEW rejects WITH (check_option = local) and row_to_json(NULL) is incorrectly accepted as NULL, while control checks like base composite row_to_json resolution and numeric array_to_json formatting behaved as expected.

❌ Failures (3)
Origin Category Severity Summary Screenshot
🔁 Regression Resolve ⚠️ High Confirmed real bug: baseline row_to_json(t) preserves schema keys, but equivalent resolver paths fail in-engine (WITH cte ... row_to_json(cte) / subquery alias) because composite type lookup only resolves persisted table types by name and cannot resolve derived aliases; explicit ::record cast from table-composite also fails because composite->record explicit cast is not implemented. N/A
🔻 Still broken (verified) Import ⚠️ High HalfCoke dump import still fails after role bootstrap because unsupported ALTER DEFAULT PRIVILEGES statements are present in the fixture SQL and are treated as fatal import errors. IMPORT-1
🔻 Still broken (verified) View 🟠 Medium CREATE VIEW rejects WITH (check_option = local) with a syntax error instead of creating the view. VIEW-6
⚠️ Derived composite row paths fail row_to_json serialization
  • What failed: Equivalent row sources should serialize consistently, but derived alias paths fail with could not create a composite type for table and explicit composite-to-record casting fails instead of producing JSON.
  • Impact: Core row_to_json(record) workflows break for common SQL patterns like CTEs and subqueries, so users cannot reliably serialize logically equivalent row sources. This creates inconsistent query behavior with no practical workaround for derived-row paths.
  • Steps to reproduce:
    1. Create a mixed-case table row fixture and verify SELECT row_to_json(t) works on the base table alias.
    2. Run equivalent CTE/subquery alias forms such as WITH cte AS (...) SELECT row_to_json(cte) and SELECT row_to_json(s) FROM (SELECT ...) s.
    3. Run SELECT row_to_json((t)::record) and compare behavior.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I traced resolver and cast handling in server/expression/table_to_composite.go, server/functions/framework/compiled_function.go, and core/casts/collection.go. The resolver only fetches persisted composite types by table name, and explicit cast lookup supports record->composite but does not provide composite->record explicit cast coverage.
  • Why this is likely a bug: The code paths accept composite-to-record compatibility intent but fail derived alias resolution and explicit cast availability, which creates inconsistent behavior for equivalent record inputs in production logic.

Relevant code:

server/expression/table_to_composite.go (lines 42-49)

// TODO: we need to get the schema, but the GMS builder doesn't have that information
typ, err := coll.GetType(ctx, id.NewType("", tableName))
if err != nil {
    return nil, err
}
if typ == nil {
    return nil, errors.New(fmt.Sprintf(`could not create a composite type for table "%s"`, tableName))
}

server/functions/framework/compiled_function.go (lines 618-625)

} else if paramType.IsRecordType() && argTypes[i].IsCompositeType() {
    // Composite types (e.g. table row types) are compatible with the generic Record parameter.
    overloadCasts[i] = casts.Cast{
        ID:       id.NewCast(argTypes[i].ID, paramType.ID),
        CastType: casts.CastType_Explicit,
        Function: id.NullFunction,
        UseInOut: false,
    }

core/casts/collection.go (lines 91-94)

// We then check for a record to composite cast
if recordCast := pgc.getRecordCast(sourceType, targetType, CastType_Explicit); recordCast.ID.IsValid() {
    return recordCast, nil
}
⚠️ HalfCoke dump fails on unsupported default privileges statements
  • What failed: Expected import to complete without intercepted query errors after role bootstrap, but import still fails because ALTER DEFAULT PRIVILEGES FOR ROLE postgres statements in the dump hit an unsupported AST path.
  • Impact: Users cannot reliably import this fixture (and similar dumps containing default-privilege statements), so a core migration/import workflow fails without a practical workaround in the product.
  • Steps to reproduce:
    1. Bootstrap ssouser and run the import flow for HalfCoke_blog_img.sql.
    2. Execute the dump through the intercept proxy import path.
    3. Observe intercepted SQL errors when default-privilege statements are parsed.
  • Stub / mock context: Authentication and role-existence checks were temporarily bypassed during this run so the import flow could proceed in an ephemeral environment and expose dump-processing behavior; the confirmed failure is independent of those bypasses because it is caused by unsupported ALTER DEFAULT PRIVILEGES handling.
  • Code analysis: I reviewed the AST handler for ALTER DEFAULT PRIVILEGES, the fixture dump contents, and the import harness error handling. The AST layer hard-returns not-supported for this statement type, the fixture includes three such statements, and the harness fails immediately when intercepted import query errors are collected.
  • Why this is likely a bug: The failing SQL is present in the shipped fixture and deterministically routes into a production code path that explicitly rejects the statement, which directly causes import failure.

Relevant code:

server/ast/alter_default_privileges.go (lines 23-26)

// nodeAlterDefaultPrivileges handles *tree.AlterDefaultPrivileges nodes.
func nodeAlterDefaultPrivileges(ctx *Context, node *tree.AlterDefaultPrivileges) (vitess.Statement, error) {
    return NotYetSupportedError("ALTER DEFAULT PRIVILEGES statement is not yet supported")
}

testing/dumps/sql/HalfCoke_blog_img.sql (lines 3774-3784)

-- Name: DEFAULT PRIVILEGES FOR SEQUENCES; Type: DEFAULT ACL; Schema: ssodb; Owner: postgres
--

ALTER DEFAULT PRIVILEGES FOR ROLE postgres IN SCHEMA ssodb GRANT ALL ON SEQUENCES TO ssouser;

--
-- Name: DEFAULT PRIVILEGES FOR FUNCTIONS; Type: DEFAULT ACL; Schema: ssodb; Owner: postgres
--

ALTER DEFAULT PRIVILEGES FOR ROLE postgres IN SCHEMA ssodb GRANT ALL ON FUNCTIONS TO ssouser;

testing/go/import_dumps_test.go (lines 645-651)

if len(allErrors) > 0 {
    t.Logf("COUNT: %d", len(allErrors))
    // If we have more than some threshold, then we'll only show the first few for ease of consumption
    for i := 0; i < len(allErrors) && i < 10; i++ {
        t.Logf("QUERY: %s\nERROR: %s", allErrors[i].Query, allErrors[i].Error)
    }
    t.FailNow()
}
🟠 Create view local check option fails to parse
  • What failed: The statement fails with a syntax error at local, but expected behavior is successful view creation and a query result row.
  • Impact: CREATE VIEW users cannot use the documented local check option form in this option syntax, so a meaningful DDL workflow fails unless users switch syntax or quoting. This causes confusing incompatibility with expected PostgreSQL behavior.
  • Steps to reproduce:
    1. Connect to doltgres with psql on localhost:5432.
    2. Run CREATE VIEW v_local_check WITH (check_option = local) AS SELECT 1 AS c;.
    3. Run SELECT * FROM v_local_check and observe the syntax error instead of a created view.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I reviewed parser grammar and CREATE VIEW AST conversion. The parser only accepts check_option assignment values as SCONST, which blocks unquoted local before conversion; the conversion layer already supports local and cascaded values once parsed.
  • Why this is likely a bug: The parser disallows the unquoted local token that the downstream CREATE VIEW conversion logic explicitly supports, creating an internal contract mismatch in production code.

Relevant code:

postgres/parser/parser/sql.y (lines 9070-9073)

view_option:
  CHECK_OPTION { $$.val = tree.ViewOption{Name: $1, CheckOpt: "cascaded"} }
| CHECK_OPTION '=' SCONST { $$.val = tree.ViewOption{Name: $1, CheckOpt: $3} }
| SECURITY_BARRIER { $$.val = tree.ViewOption{Name: $1, Security: false} }

server/ast/create_view.go (lines 41-49)

switch strings.ToLower(opt.Name) {
case "check_option":
	switch strings.ToLower(opt.CheckOpt) {
	case "local":
		checkOption = tree.ViewCheckOptionLocal
	case "cascaded":
		checkOption = tree.ViewCheckOptionCascaded
	default:
		return nil, errors.Errorf(`"ERROR:  syntax error at or near "%s"`, opt.Name)
	}
✅ Verified Passing (3)
Category Summary Screenshot
Array Decimal array elements were serialized as JSON numbers ([1.25,2.50]) without string quoting. N/A
Record The problematic composite-subquery invocation was rejected consistently during resolution with a stable error, while normal composite invocation executed successfully; no late Eval cast metadata failure was observed. N/A
Resolve row_to_json on rtj_test composite row resolved and executed successfully, returning JSON output without function resolution errors. N/A
↪️ Inherited from Prior Run (12)

Tests that passed in the prior run. c3 judged them unaffected by the diff and did not retest.

Category Summary Screenshot
Array array_to_json(ARRAY[1,NULL,3]) returned [1,null,3] as expected. N/A
Import Missing setup role is reported clearly during import interception. N/A
Import Malicious privilege escalation statement is rejected without lasting elevation. N/A
Import Immediate retry after partial setup failure remains stable with consistent diagnostics. N/A
Render After seeding rtj_test with Alice/Bob, SELECT row_to_json(t) FROM rtj_test AS t WHERE id=1 returned {"id":1,"name":"Alice"}, confirming named-column keys (id,name) rather than positional keys (f1,f2). N/A
Resolve Executing SELECT row_to_json(1); produced the expected function-resolution error (row_to_json(integer) does not exist), confirming scalar input is not silently coerced. N/A
Resolve SELECT row_to_json(row(1, true, null), true) succeeded and returned JSON with keys f1/f2/f3, value 1 for f1, true for f2, and JSON null for f3. N/A
Resolve Controlled nested row_to_json error handling was observed and the SQL session remained responsive. N/A
Resolve Compile-time and runtime row_to_json behavior aligned after re-running against the source-built server. N/A
Resolve Composite, record, and scalar invocations produced consistent explicit errors without destabilizing the session. N/A
View CREATE VIEW with security_barrier=false succeeded and querying v_sb_false returned c=1. N/A
View CREATE VIEW with security_barrier=true was rejected and v_sb_true was not created. N/A
⚠️ Additional Findings (1)

These findings are unrelated to the current changes but were observed during testing.

Origin Category Severity Summary Screenshot
🆕 New Record 🟠 Medium Confirmed real bug: SELECT row_to_json(NULL); resolves and returns NULL instead of emitting a function-resolution error for unknown NULL input. N/A
🟠 Unknown NULL input bypasses row_to_json resolution checks
  • What failed: The call resolves and returns SQL NULL, but this case should be rejected at resolution for unknown NULL input lacking explicit record type information.
  • Impact: Type-resolution rules become overly permissive, allowing invalid row_to_json calls to pass silently as NULL instead of producing deterministic errors. This can hide query mistakes and make behavior less predictable for client code.
  • Steps to reproduce:
    1. Connect to the local database and run SELECT row_to_json(NULL) without any explicit cast.
    2. Observe function resolution and evaluation result.
    3. Compare with expected resolver rejection for unknown NULL without record typing.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I reviewed server/functions/row_to_json.go, server/functions/framework/compiled_function.go, and core/casts/collection.go. The function is strict (returns NULL on NULL args), and unknown is globally treated as implicitly castable, so overload resolution accepts unknown NULL then strict short-circuits instead of rejecting the call.
  • Why this is likely a bug: The implementation accepts unknown NULL through implicit casting and strict-null short-circuiting, which bypasses expected resolver rejection for an untyped record argument.

Relevant code:

server/functions/row_to_json.go (lines 35-40)

var row_to_json_record = framework.Function1{
    Name:       "row_to_json",
    Return:     pgtypes.Json,
    Parameters: [1]*pgtypes.DoltgresType{pgtypes.Record},
    Strict:     true,

server/functions/framework/compiled_function.go (lines 295-297)

if args[i] == nil && isStrict {
    return nil, nil
}

core/casts/collection.go (lines 189-196)

// It is always valid to convert from the `unknown` type
if sourceType.ID == pgtypes.Unknown.ID {
    return Cast{
        ID:       castID,
        CastType: CastType_Implicit,
        Function: id.NullFunction,
        UseInOut: true,

View Full Run


Tell us how we did: Give Ito Feedback

@jennifersp jennifersp merged commit 234a37b into main May 21, 2026
22 checks passed
@jennifersp jennifersp deleted the jennifer/fixes branch May 21, 2026 17:49
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.

2 participants