Repository navigation
Fix accessing an inherited property through subcollection - #580
Conversation
john-sanchez31
changed the title
|
|
||
| # Wait until server is ready | ||
| for _ in range(20): | ||
| if proc.poll() is not None: |
There was a problem hiding this comment.
Unrelated changes to the mock server setup error handling. I was already using port 8000 but didn't get the correct error message
hadia206
left a comment
hadia206
left a comment
There was a problem hiding this comment.
Good investigation and solution John.
I have minor comments that would like to get addressed before approving
| output, _ = proc.communicate() | ||
| raise RuntimeError(f"Mock server failed to start:\n{output}") |
There was a problem hiding this comment.
This should be in try/except with a timeout to avoid hanging.
try:
output, _ = proc.communicate(timeout=5)
except subprocess.TimeoutExpired:
proc.kill()
output, _ = proc.communicate()
raise RuntimeError(f"Mock server failed to start:\n{output}")
| .WHERE(key <= 3) | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Let's add another test for PARTITION-child with context_inherits being False as written in the comments in the code.
Accesses are still valid when the term comes from the child's own
data, e.g. a CALCULATE inside the child expression
(`customer.CALCULATE(customer_name=name).orders.customer_name`) or
inside the data of a PARTITION child (`pricing_collection.price`).
e.g. something like the pricing_collection.price case mentioned above. Right now only the "shadowed" and "plain error" branches are tested.
knassre-bodo
left a comment
knassre-bodo
left a comment
There was a problem hiding this comment.
I have one structural change I'd prefer to how the names are found.
| def names_defined_in_child_chain(self, unqualified: UnqualifiedNode) -> set[str]: | ||
| """ | ||
| Walks the unqualified collection chain of a child access (e.g. | ||
| `customer.WHERE(...).CALCULATE(customer_name=name).orders.BEST(...)`) | ||
| down to the root, collecting every term name defined by a CALCULATE | ||
| that is part of the chain itself. | ||
|
|
||
| Args: | ||
| `unqualified`: the unqualified collection node to walk. | ||
|
|
||
| Returns: | ||
| The set of term names defined by CALCULATEs within the chain. | ||
| """ | ||
| defined: set[str] = set() | ||
| node: UnqualifiedNode = unqualified | ||
| while not isinstance(node, UnqualifiedRoot): | ||
| if isinstance(node, UnqualifiedCalculate): | ||
| for term_name, _ in node._parcel[1]: | ||
| defined.add(term_name) | ||
| predecessor = node._parcel[0] | ||
| if not isinstance(predecessor, UnqualifiedNode): | ||
| break | ||
| node = predecessor | ||
| return defined |
There was a problem hiding this comment.
I think this is a more dangerous/complex way to handle the search for legal names. I think a version that traverses the QDAG nodes would be safer, since those APIs are more well defined for this sort of interaction.
knassre-bodo
left a comment
knassre-bodo
left a comment
There was a problem hiding this comment.
LGTM, just need to resolve merge conflicts, re-run CI, then go ahead and merge 👍
3 participants
Linked ticket
Closes #564
Type of change
What changed and why?
Fix for the bug where a subcollection accessing a inherited term qualify correctly but later raises an error. Adding a new verification on qualification that makes sure that the reference being qualified is actually part of the subcollection where is being accessed from.
How I tested this?
parent_calc_access_errorthat raises the corresponding errorshadowed_calc_term_exethat covers the possibility of a name being shadow in the child which makes its access valid.Notes for reviewers
Adding better error handling for the mock server setup