Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Optional links query doesn't return anything #1128

Open
Hades32 opened this issue Dec 10, 2021 · 22 comments · Fixed by #1145
Open

Optional links query doesn't return anything #1128

Hades32 opened this issue Dec 10, 2021 · 22 comments · Fixed by #1145
Assignees
Labels
bug Something isn't working

Comments

@Hades32
Copy link
Contributor

Hades32 commented Dec 10, 2021

The below query doesn't return anything although the anyOf contains true and the id exists. Removing the links stuff returns the one contract as expected

{
    "type": "object",
    "properties": {
        "id": {
            "const": "b681c25a-7bd9-4b24-b2cc-c540287c8950"
        }
    },
    "anyOf": [
        {
            "$$links": {
                "was built into": {
                    "type": "object",
                    "anyOf": [
                        {
                            "$$links": {
                                "was built into": {
                                    "type": "object",
                                    "anyOf": [
                                        {
                                            "$$links": {
                                                "was built into": {
                                                    "type": "object"
                                                }
                                            }
                                        },
                                        {
                                            "$$links": {
                                                "was merged as": {
                                                    "type": "object"
                                                }
                                            }
                                        },
                                        true
                                    ]
                                }
                            }
                        },
                        {
                            "$$links": {
                                "was merged as": {
                                    "type": "object"
                                }
                            }
                        },
                        true
                    ]
                }
            }
        },
        {
            "$$links": {
                "was merged as": {
                    "type": "object"
                }
            }
        },
        true
    ]
}
@Hades32
Copy link
Contributor Author

Hades32 commented Dec 10, 2021

@Ereski I think we had talked about something similar before, so this might be a dup, but I couldn't find the relevant issue

@Ereski
Copy link
Contributor

Ereski commented Dec 10, 2021

@Hades32 this is the same query as the another issue you opened regarding PG erroing out due to long identifiers? Seems like it's closed now?

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 10, 2021

@Ereski IIRC the one with long identifiers just errored out. This one returns an empty results array

@Ereski
Copy link
Contributor

Ereski commented Dec 10, 2021

I can't find that issue. Guess it got deleted when jf went public. The query looks ok, I'll take a closer look 👍

@Hades32 Just FYI, { "type": "object" } can be replaced with true.

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 16, 2021

@Ereski turns out this is preventing the graph at https://jel.ly.fish/check-run-719ae387b67014dce33976fb299fe940f4d8dccf (and a few others. Not sure yet, what the pattern is) to show up.
Could you give this some priority?

@Ereski
Copy link
Contributor

Ereski commented Dec 16, 2021

@Hades32 for this specific card, what's the relevant part of its link graph for the query? Which links exist and which don't?

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 16, 2021

@Ereski the first level of was built into exists but the second level doesn't

@Ereski
Copy link
Contributor

Ereski commented Dec 16, 2021

@Hades32 just to confirm, it's something like this:

The root (id: b681c25a-7bd9-4b24-b2cc-c540287c8950):

  • Has no was merged as link.
  • Has a single was built into link. This link in turn has no was built into nor was merged as links.

Correct?

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 17, 2021

exactly @Ereski

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

Reproduced. Gonna take a closer look at the query

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

The query looks fine 🤔

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

It works if I remove the middle anyOf

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

SQL is just a bunch of left joins and the only constraint is the root card ID 🤔

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

I'm stumped @Hades32. Not sure what's going on here. The only differences I can see so far are things that don't matter.

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 17, 2021

Then it must be really bad :/
Did you reproduce it locally or on prod? I'm wondering if there's bad data in prod for some reason

@Ereski
Copy link
Contributor

Ereski commented Dec 17, 2021

Locally. I wrote a test.

I'm now wondering if the left joins are interacting in a weird way. Gonna run some tests monday. Do you know of any other query we currently have with an optional link inside another optional link @Hades32?

@Hades32
Copy link
Contributor Author

Hades32 commented Dec 20, 2021

@Ereski I can't think of any that exist right now

@Ereski
Copy link
Contributor

Ereski commented Dec 20, 2021

Reduced to:

{
    type: 'object',
    properties: {
        id: {
            const: "b681c25a-7bd9-4b24-b2cc-c540287c8950",
        }
    },
    $$links: {
        'was built into': {
            type: 'object',
            anyOf: [
                {
                    $$links: {
                        'was built into': {
                            type: 'object',
                            anyOf: [
                                {
                                    $$links: {
                                        'was built into': {
                                            type: 'object',
                                        },
                                    },
                                },
                                true,
                            ],
                        },
                    },
                },
                true,
            ],
        },
    },
},

@Ereski
Copy link
Contributor

Ereski commented Dec 20, 2021

So far it seems the requirement is: at least three levels of $$links, where the last two are optional. Def something weird with those left joins. They have some peculiar corner cases that I forget, so I need to look into that first.

@Ereski
Copy link
Contributor

Ereski commented Dec 20, 2021

Yep, I think I found the problem. The part of the query that builds a full JSON object from a list of link edges doesn't seem to deal very well with a corner case when aggregate functions are used with left joins that return no matches. Only happens with two nested, optional $$links because that introduces an inner join that ends up failing because it tries to compare to null, making the query return nothing.

@Ereski Ereski self-assigned this Dec 20, 2021
@Ereski Ereski added the bug Something isn't working label Dec 20, 2021
Ereski added a commit that referenced this issue Dec 20, 2021
The subquery that builds the final contract tree when querying with
links uses an inner join to get the nested link. Roughly:

```
JOIN (
  // Build the linked contract...
) AS linked
ON edge.source = linked.id
```

These joins are nested in the case of nested links and there is a
toplevel `LATERAL`. In the case of two nested optional links, the
`LATERAL` subquery looks roughly like this:

```
FROM
  cards,
  LATERAL (
    // ...
    JOIN (
      // ...
    ) AS linked
    ON edge1.source = linked.id
    // ...
  )
WHERE edge0.source = cards.id
```

If the optional contracts don't exist, `linked.id` will be null and the
`edge1.to = linked.id` condition will fail. As a consequence, the whole
lateral will be empty and so will the result. The fix is change those
inner joins into left joins.

This is a tentative fix because I have fuzzy memories of making this
exact change and causing unrelated tests to fail.

Fixes #1128
Change-type: patch
Signed-off-by: Carol Schulze <carol@balena.io>
Ereski added a commit that referenced this issue Dec 20, 2021
The subquery that builds the final contract tree when querying with
links uses an inner join to get the nested link. Roughly:

```
JOIN (
  // Build the linked contract...
) AS linked
ON edge.source = linked.id
```

These joins are nested in the case of nested links and there is a
toplevel `LATERAL`. In the case of two nested optional links, the
`LATERAL` subquery looks roughly like this:

```
FROM
  cards,
  LATERAL (
    // ...
    JOIN (
      // ...
    ) AS linked
    ON edge1.source = linked.id
    // ...
  )
WHERE edge0.source = cards.id
```

If the optional contracts don't exist, `linked.id` will be null and the
`edge1.to = linked.id` condition will fail. As a consequence, the whole
lateral will be empty and so will the result. The fix is change those
inner joins into left joins.

This is a tentative fix because I have fuzzy memories of making this
exact change and causing unrelated tests to fail.

Fixes: #1128
Change-type: patch
Signed-off-by: Carol Schulze <carol@balena.io>
@ghost ghost closed this as completed in #1145 Dec 20, 2021
ghost pushed a commit that referenced this issue Dec 20, 2021
@Hades32
Copy link
Contributor Author

Hades32 commented Dec 21, 2021

Unfortunately, it doesn't seem like this fixed the issue. Neither https://jel.ly.fish/check-run-697118058f5b4e4d962e38325137e40ff6655578 nor https://jel.ly.fish/check-run-719ae387b67014dce33976fb299fe940f4d8dccf are showing a graph, as the query returns empty.

Tested on JF v42.2.73 which includes core v8.2.7

@Hades32 Hades32 reopened this Dec 21, 2021
@Ereski
Copy link
Contributor

Ereski commented Dec 21, 2021

Something is wrong. JF shouldn't even compile with any core version higher than 8.2.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants