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

feat(frontend): support single dimension array for extended-mode query #15614

Closed
wants to merge 6 commits into from

Conversation

jetjinser
Copy link
Contributor

@jetjinser jetjinser commented Mar 11, 2024

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Split e2e_test/batch/types/array_ty.slt.part into 1-dim e2e_test/batch/types/array_1d_ty.slt.part and multi-dim (keep original name).

Partially resolved - #7949.

import psycopg

def ep(query):
    cur.execute(query, binary=True)  # type: ignore
    for row in cur:
        print(row)
    print()

conn = psycopg.connect(host="localhost", port="4566", dbname="dev", user="root")
with conn.cursor() as cur:
    int_tys = ["int2", "int4", "int8", "rw_int256", "numeric"]
    flt_tys = ["float4", "float8"]
    txt_tys = ["text", "bytea"]

    for it in int_tys:
        query = f"select Array[1::{it}, 2::{it}, 3::{it}]"
        ep(query)

    for ft in flt_tys:
        query = f"select Array[1.1::{ft}, 2.2::{ft}, 3.3::{ft}]"
        ep(query)

    for tt in txt_tys:
        query = f"select Array['a'::{tt}, 'b'::{tt}, 'c'::{tt}]"
        ep(query)

    # error: n-dim where n > 1
    # query = "select Array[Array[1, 2, 3], Array[4, 5, 6]]"
    # ep(query)

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

@neverchanje
Copy link
Contributor

Thanks @xiangjinwu PTAL

src/common/src/types/to_binary.rs Outdated Show resolved Hide resolved
where
Self: Sized,
{
self.iter().collect_vec().to_sql(ty, out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure only 1 dimension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.rs/postgres-types/0.2.6/postgres_types/trait.ToSql.html#arrays

DataType::Struct(_) | DataType::List(_) => bail_not_implemented!(
issue = 7949,
"the pgwire extended-mode encoding for lists with more than one dimension ({ty}) is unsupported"
),

I think it should be guaranteed here that there will only be an array of 1 dimension?

Copy link
Contributor

Choose a reason for hiding this comment

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

But as a general trait implementation, to_sql may have other callers (in the future) in addition to to_binary.

Copy link
Contributor Author

@jetjinser jetjinser Mar 14, 2024

Choose a reason for hiding this comment

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

I rechecked, the postgres-types checked the array dimensions, so it was handled correctly:
https://github.com/sfackler/rust-postgres/blob/e528e01bbe2f7e198a55112fd6de3a24db3b8a6e/postgres-types/src/lib.rs#L586-L588

If the array dimension is greater than 1, it will return Err with error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation looks fine to me. I don't see buggy points.

Copy link
Contributor

Choose a reason for hiding this comment

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

The link above is for FromSql. There seems to be no check of multiple dimensions in ToSql there:
https://github.com/sfackler/rust-postgres/blob/e528e01bbe2f7e198a55112fd6de3a24db3b8a6e/postgres-types/src/lib.rs#L960

Can we confirm by actually executing to_sql on a Vec<Vec<i32>> or ListRef of ListRef of i32 value?

@jetjinser jetjinser requested a review from xiangjinwu March 14, 2024 10:23
where
Self: Sized,
{
matches!(ty.kind(), postgres_types::Kind::Array(_))
Copy link
Contributor

@neverchanje neverchanje Mar 15, 2024

Choose a reason for hiding this comment

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

Does it mean that INT[] can be accpeted by DATE[]?
You should check the inner type as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think accept has a way to check the inner type... it only know the postgres_type::Type.
It should be a limitation of the ToSql api.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think accept has a way to check the inner type... it only know the postgres_type::Type. It should be a limitation of the ToSql api.

What's _ inside the postgres_types::Kind::Array(_) then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I understand, but we can still only check whether ty is a one-dimensional array. Is this implementation correct?

@jetjinser jetjinser enabled auto-merge March 22, 2024 06:16
@jetjinser jetjinser disabled auto-merge March 22, 2024 06:16
Copy link
Contributor

This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review.

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

Successfully merging this pull request may close these issues.

3 participants