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

Remove check for scalar(vertex,edge or path) agtypes in unwind #736

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

MuhammadTahaNaveed
Copy link
Member

  • Removed check that was disabling unwind to be used on arrays with elements of type vertex,edge or path.
  • Added regression tests for unwind.

@emotionbug
Copy link
Contributor

If you think the block_types variable is no longer needed, please modify the code associated with it as well.

age/age--1.2.0.sql

Lines 4165 to 4170 in 6e8836b

CREATE FUNCTION ag_catalog.age_unnest(agtype, block_types boolean = false)
RETURNS SETOF agtype
LANGUAGE c
IMMUTABLE
PARALLEL SAFE
AS 'MODULE_PATHNAME';

funcexpr = ParseFuncOrColumn(pstate, unwind->funcname,
list_make2(expr, makeBoolConst(true, false)),
pstate->p_last_srf, unwind, false,
target_syntax_loc);

@MuhammadTahaNaveed
Copy link
Member Author

Hi @emotionbug,

I did not modify those after seeing this comment. Screenshot_2023-03-09-12-09-50-77_320a9a695de7cdce83ed5281148d6f19.jpg

What do you think, should it be modified?

@emotionbug
Copy link
Contributor

I think that variable is for the constraint you removed.
Since you've already determined that the constraint is unnecessary and removed it, I think the comments and code around it should also be modified.

Because even if you don't, it's already doing nothing with the variable, and it's not what the comment says.

@MuhammadTahaNaveed
Copy link
Member Author

I think that variable is for the constraint you removed.
Since you've already determined that the constraint is unnecessary and removed it, I think the comments and code around it should also be modified.

Because even if you don't, it's already doing nothing with the variable, and it's not what the comment says.

Ok thanks. I will update the PR.

@jrgemignani
Copy link
Contributor

jrgemignani commented Mar 9, 2023

@MuhammadTahaNaveed I'm not sure why your commit description comments say Co-authored-by you?

- Added regression tests for unwind.
@MuhammadTahaNaveed
Copy link
Member Author

@MuhammadTahaNaveed I'm not sure why your commit description comments say Co-authored-by you?

@jrgemignani Fixed it.

@jrgemignani jrgemignani merged commit 16d9c08 into apache:master Mar 11, 2023
@MuhammadTahaNaveed MuhammadTahaNaveed deleted the unwind-dev branch March 11, 2023 05:11
jrgemignani pushed a commit that referenced this pull request Mar 20, 2023
- Added regression tests for unwind.
jrgemignani pushed a commit that referenced this pull request Mar 28, 2023
- Added regression tests for unwind.
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.

3 participants