ahujaanmol1289 commented on PR #70715: URL: https://github.com/apache/airflow/pull/70715#issuecomment-5140896899
Thanks for the thorough review — all three points are correct, and I appreciate the detailed analysis. **On the `hash()` regression**: You're right. I misread the original code — `__init__` dumps the unsorted dict via `json.dumps(dag_data, sort_keys=True)`, so the sort only happens once (inside `hash()`), not twice. Making `hash()` delegate to `_compute_hash_and_storage_json()` forces the unchanged-DAG path to generate and discard storage JSON it never needed. That's a net regression on the hot path. **On the storage bytes change:** Also correct. `_sort_serialized_dag_dict()` reorders lists (tasks by task_id, all-string lists alphabetically), which `json.dumps(sort_keys=True)` alone does not. Dumping from the sorted dict changes the stored bytes for any DAG with unsorted lists. The `fileloc: None` edge case is a real bug too — the `is not None` guard would silently drop it from storage. I should have caught both of these. **On the test style:** Will fix — `spec=LazyDeserializedDAG` on mocks, imports to file top, and simplifying the `__new__` + `__init__` pattern. Your suggestion to pass the precomputed hash into the constructor instead is the right fix — it removes the actual duplication (write_dag hashing and then cls(dag) hashing again when the DAG changed) without touching the unchanged-DAG path or altering storage bytes. I'll rework the PR to take that approach. Should I push a revised commit to this branch, or would you prefer I close this and open a fresh PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
