chore: accept a counter option in prepare_query/3#4714
chore: accept a counter option in prepare_query/3#4714josevalim merged 5 commits intoelixir-ecto:masterfrom
counter option in prepare_query/3#4714Conversation
zachdaniel
force-pushed
the
merge-support
branch
from
1224931 to
386f47f
Compare
|
Out of curiosity what problem does this solve? |
|
@zachdaniel is implementing |
lib/ecto/query/planner.ex
Outdated
| {query, params, key} = plan(query, operation, adapter) | ||
| {cast_params, dump_params} = Enum.unzip(params) | ||
| key = if query_cache?, do: key, else: :nocache | ||
| key = if query_cache?, do: {key, counter}, else: :nocache |
There was a problem hiding this comment.
@zachdaniel I thought it would be better to always add the counter, to reduce the number of branches, but it seems that either using a list is wrong or we are messing with the key in an incompatible way. Can you please take a deeper look?
There was a problem hiding this comment.
Sorry I'm not quite following.
it seems that either using a list is wrong or we are messing with the key in an incompatible way.
WDYM by using a list?
Is this about the test failures? The seem like timeouts in the GH actions to me but maybe I'm missing something.
There was a problem hiding this comment.
I am not so sure. I have cloned ecto, pulled your branch, rebased master, and ECTO_PATH=../ecto ECTO_ADAPTER=pg mix test in ecto_sql fails for me.
And I decided to always return a tuple with the counter so we don't have hidden behaviour depending if counter is zero or not.
There was a problem hiding this comment.
I will do the same and investigate 👌
There was a problem hiding this comment.
@josevalim I could be missing something, but when I do the same I get all tests passing 🤔
There was a problem hiding this comment.
What did you run and which project? CI is also failing but it was not failing before my changes (and passed with yours).
There was a problem hiding this comment.
Sorry. I understand now. I didn't realize you had pushed to the branch 🤦♂️. The CI failures here seem like timeouts and my branch passed ecto_sql tests locally but I didn't pull. Will investigate 😄
There was a problem hiding this comment.
Okay so the problem is that the system uses the sentinal key :nocache, but that was getting wrapped in a tuple.
|
💚 💙 💜 💛 ❤️ |
3 participants
No description provided.