Ensures DAG params order regardless of backend #40156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Instead of needing an extra key and extra nesting, should we just serialise params as list of key-value pairs instead? |
|
Alternatively maybe we can serialise the key order under a separate key? This is proably slightly more compact in most cases as well. |
I like this. I disregarded it previously, because I thought it would imply changing the MySQL at least seems to hold true to the standard :)
|
|
Serializing as a list would require some additional handling of already serialized DAGs (using a dict) though |
Fixes apache#40154 This change adds an extra attribute to the serialized DAG param objects which helps us decide the order of the deserialized params dictionary later even if the backend messes with us. I decided not to limit this just to MySQL since the operation is inexpensive and may turn out to be helpful. I made sure the new test fails with the old implementation + MySQL. I assume this test will be executed with MySQL somewhere in the build actions?
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Usiel
force-pushed
the
usiel/fix-dag-param-order-with-mysql
branch
from
aa2af24 to
6da5cbe
Compare
uranusjr
reviewed
Jun 11, 2024
uranusjr
reviewed
Jun 11, 2024
Usiel
force-pushed
the
usiel/fix-dag-param-order-with-mysql
branch
from
6da5cbe to
8a0c5fb
Compare
Alternative to previous approach: We serialize the DAG params dict as a list of tuples which _should_ keep their ordering regardless of backend. Backwards compatibility is ensured because if `encoded_params` is a `dict` (not the expected `list`) then `dict(encoded_params)` still works.
|
Apologies for the noise, was fighting with a test. I'll push another version in a bit with the incorporated comments. |
Based on suggestions by @uranusjr with an additional fix to make mypy happy.
Usiel
force-pushed
the
usiel/fix-dag-param-order-with-mysql
branch
from
8a0c5fb to
a1c7e31
Compare
|
Ok, done. Latest push is rebased on |
uranusjr
approved these changes
Jun 11, 2024
Lee-W
approved these changes
Jun 11, 2024
eladkal
approved these changes
Jun 13, 2024
utkarsharma2
pushed a commit
that referenced
this pull request
Jul 2, 2024
* Ensures DAG params order regardless of backend Fixes #40154 This change adds an extra attribute to the serialized DAG param objects which helps us decide the order of the deserialized params dictionary later even if the backend messes with us. I decided not to limit this just to MySQL since the operation is inexpensive and may turn out to be helpful. I made sure the new test fails with the old implementation + MySQL. I assume this test will be executed with MySQL somewhere in the build actions? * Removes GitHub reference Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> * Serialize DAG params as array of tuples to ensure ordering Alternative to previous approach: We serialize the DAG params dict as a list of tuples which _should_ keep their ordering regardless of backend. Backwards compatibility is ensured because if `encoded_params` is a `dict` (not the expected `list`) then `dict(encoded_params)` still works. * Make backwards compatibility more explicit Based on suggestions by @uranusjr with an additional fix to make mypy happy. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> (cherry picked from commit 2149b4d)
35 tasks
romsharon98
pushed a commit
to romsharon98/airflow
that referenced
this pull request
Jul 26, 2024
* Ensures DAG params order regardless of backend Fixes apache#40154 This change adds an extra attribute to the serialized DAG param objects which helps us decide the order of the deserialized params dictionary later even if the backend messes with us. I decided not to limit this just to MySQL since the operation is inexpensive and may turn out to be helpful. I made sure the new test fails with the old implementation + MySQL. I assume this test will be executed with MySQL somewhere in the build actions? * Removes GitHub reference Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com> * Serialize DAG params as array of tuples to ensure ordering Alternative to previous approach: We serialize the DAG params dict as a list of tuples which _should_ keep their ordering regardless of backend. Backwards compatibility is ensured because if `encoded_params` is a `dict` (not the expected `list`) then `dict(encoded_params)` still works. * Make backwards compatibility more explicit Based on suggestions by @uranusjr with an additional fix to make mypy happy. --------- Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #40154
This change adds an extra attribute to the serialized DAG param objects which helps us decide the order of the deserialized params dictionary later even if the backend messes with us.
I decided not to limit this just to MySQL since the operation is inexpensive and may turn out to be helpful (edit: Postgres is also affected).
Tests
I made sure the new test fails with the old implementation + MySQL/Postgres. I assume this test will be executed with MySQL somewhere in the build actions?