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

DML in cast_expr #6714

Closed
wants to merge 5 commits into from
Closed

DML in cast_expr #6714

wants to merge 5 commits into from

Conversation

aljazerzen
Copy link
Contributor

Closes #5050

@aljazerzen aljazerzen changed the title DML in cast_expr GH job to check that postgres/ has not been unintentianally changed, take 2 Jan 19, 2024
@aljazerzen aljazerzen changed the title GH job to check that postgres/ has not been unintentianally changed, take 2 DML in cast_expr Jan 19, 2024
@aljazerzen aljazerzen force-pushed the cast-expr-dml branch 2 times, most recently from 2126f4c to 7ed91c0 Compare January 19, 2024 16:03
@aljazerzen aljazerzen marked this pull request as draft January 19, 2024 16:03
if (path_id, 'iterator') in stmt.path_rvar_map:
return

pathctx.put_path_var(stmt, path_id, id_expr, aspect='iterator')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msullivan I'm not sure if this is the correct way to do it.

I had a problem where DML within iteration was producing this:

    WITH
    source_rel AS
        ((
            SELECT *, ctid FROM "the_link_table"
            -- this ctid should be used as the iterator
        )),
    "iter~1" AS
        ((
            FROM source_rel AS "source_rel~1"
            CROSS JOIN LATERAL (
                SELECT/*<pg.SelectStmt at 0x7f82c0915f10>*/
                    "source_rel~1"."hell" AS "hell_value~1",
                    edgedb.uuid_generate_v4() AS "hell_iterator~1"
                    -- this uuid_generate call should not be here.
                    -- ctid should be used instead.
                    -- I inject its path and rel vars so it would be picked up
                    -- but the code above overriddes it.
                    -- So I've disabled that.
                WHERE
                    ("source_rel~1"."hell" IS NOT NULL)
            ) AS "Hello~3"
            SELECT/*<pg.SelectStmt at 0x7f82c0914110>*/
                "Hello~3"."hell_iterator~1" AS "hell_iterator~2"
        )),
    "ins_contents~1" AS
        (...)
    ...

Copy link
Member

Choose a reason for hiding this comment

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

This seems plausible, though I'll ponder it a bit.
(I would restructure it so that the put_path_var is in the conditional instead of having an early return.)

external_rels[tgt_path_id] = \
(src_rel, ('identity', 'source', 'value', 'iterator'))

# if ptr_table:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I have the following DDL:

alter type Base {
    alter link foo {
        alter property bar set type int32;
    };
};

I set external_rels for Base, Base::foo and Base::foo@.
But on lookup for set with path_id (default::Base).>foo[IS default::Child]@bar[IS std::int64], I'm not finding it in external_rels.

This should happen here: https://github.com/edgedb/edgedb/blob/7a145a9596fd21c4d5a330839459947e0582e79a/edb/pgsql/compiler/relgen.py#L426-L428

Copy link
Member

Choose a reason for hiding this comment

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

Huh, you mean external_rels is getting cleared somewhere?

msullivan added a commit that referenced this pull request Jan 30, 2024
That causes us to have lots of incorrect 'iterator' aspects in rvar
maps.  Instead, just properly populate the aspects explicitly at the
one relevant call site.

This might help with #6714.
msullivan added a commit that referenced this pull request Jan 31, 2024
That causes us to have lots of incorrect 'iterator' aspects in rvar
maps.  Instead, just properly populate the aspects explicitly at the
one relevant call site.

This might help with #6714.
aljazerzen pushed a commit that referenced this pull request Mar 12, 2024
That causes us to have lots of incorrect 'iterator' aspects in rvar
maps.  Instead, just properly populate the aspects explicitly at the
one relevant call site.

This might help with #6714.
@aljazerzen
Copy link
Contributor Author

I give up. I might come back when I understand how our backend compiler works better.

@aljazerzen aljazerzen closed this Apr 3, 2024
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.

Support DML in cast_expr
2 participants