You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/02/24 21:55:32 UTC
[GitHub] [airflow] dstandish opened a new pull request #21806: Rename `dagrun_id` to `dag_run_id`
dstandish opened a new pull request #21806:
URL: https://github.com/apache/airflow/pull/21806
I think we can call this attribute `dag_run_id` without suffering too much ambiguity, principally because _dag_ does not, strictly speaking, have a `run_id`; it's the dag _run_ that has a `run_id`.
For a long time, the codebase has used `dag_run_id` or "dagrun id" to refer to the dagrun.run_id column.
As of recently, we now have `dagrun_id` alongside `dag_run_id` -- the former is supposed to refer to `DagRun.id` and the latter to `DagRun.run_id`. But personally I think that retains too much ambiguity. When reading it, you're not sure if it maybe is just a typo, or what exactly it is / how it is different from dag_run_id. And it doesn't obey standard camel-to-snake conversion.
I think we can do a better job of reducing ambiguity by using, wherever possible, `run_id` to mean `DagRun.run_id` and `dag_run_id` to mean the integer PK. (i don't think we have to resort to the painfully verbose `dag run run id`).
So, this PR hopes to rename `dagrun_id` to `dag_run_id`, and tries to replace any old references to "dag run id" to simply "run id".
The one wrinkle is the API. There are references to "dag_run_id". What I've done here is leave that alone. We could think of the DagRun.id as internal, and not user-facing from the rest API perspective. We could consider deprecating / renaming this param, but I'm considering that out of scope since it's already been out there and is unaffected by the recent changes (which this PR hopes to address before they go out in 2.3.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish merged pull request #21806:
URL: https://github.com/apache/airflow/pull/21806
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r814601163
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
I think I'm with TP on this. Yes the table is "dag_run", but by keeping this as "dagrun_id" it is (marginally) clearer that this is the "id" column of the "dagrun" table.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815261308
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
Ah i see we're talking about the column in the xcom table, which is pointing to the dag run table.
The PK in dag_run is already `id`.
So yeah, same reasoning essentially applies. When we refer to id col of a table, it should be `f"{table_name}_{col_name}"`. I don't think there's really any reason to diverge from this convention here. There's no "run_id" on the "dag" table so there's no conflict here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815261308
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
Ah i see we're talking about the column in the xcom table, which is pointing to the dag run table.
The PK in dag_run is already `id`.
So yeah, same reasoning essentially applies. When we refer to id col of a table, it should be f"{table_name}_{col_name}". I don't think there's really any reason to diverge from this convention here. There's no "run_id" on the "dag" table so there's no conflict here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r816508194
##########
File path: airflow/models/xcom.py
##########
@@ -160,26 +160,26 @@ def set(
message = "Passing 'execution_date' to 'XCom.set()' is deprecated. Use 'run_id' instead."
warnings.warn(message, DeprecationWarning, stacklevel=3)
try:
- dagrun_id, run_id = (
+ dag_run_id, run_id = (
session.query(DagRun.id, DagRun.run_id)
.filter(DagRun.dag_id == dag_id, DagRun.execution_date == execution_date)
.one()
)
except NoResultFound:
raise ValueError(f"DAG run not found on DAG {dag_id!r} at {execution_date}") from None
- elif run_id == IN_MEMORY_DAGRUN_ID:
- dagrun_id = -1
+ elif run_id == IN_MEMORY_RUN_ID:
+ dag_run_id = -1
else:
- dagrun_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
- if dagrun_id is None:
+ dag_run_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
+ if dag_run_id is None:
raise ValueError(f"DAG run not found on DAG {dag_id!r} with ID {run_id!r}")
value = cls.serialize_value(
value=value,
key=key,
task_id=task_id,
dag_id=dag_id,
- run_id=dagrun_id,
+ run_id=dag_run_id,
Review comment:
```suggestion
run_id=run_id,
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r816306151
##########
File path: airflow/models/xcom.py
##########
@@ -160,26 +160,26 @@ def set(
message = "Passing 'execution_date' to 'XCom.set()' is deprecated. Use 'run_id' instead."
warnings.warn(message, DeprecationWarning, stacklevel=3)
try:
- dagrun_id, run_id = (
+ dag_run_id, run_id = (
session.query(DagRun.id, DagRun.run_id)
.filter(DagRun.dag_id == dag_id, DagRun.execution_date == execution_date)
.one()
)
except NoResultFound:
raise ValueError(f"DAG run not found on DAG {dag_id!r} at {execution_date}") from None
- elif run_id == IN_MEMORY_DAGRUN_ID:
- dagrun_id = -1
+ elif run_id == IN_MEMORY_RUN_ID:
+ dag_run_id = -1
else:
- dagrun_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
- if dagrun_id is None:
+ dag_run_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
+ if dag_run_id is None:
raise ValueError(f"DAG run not found on DAG {dag_id!r} with ID {run_id!r}")
value = cls.serialize_value(
value=value,
key=key,
task_id=task_id,
dag_id=dag_id,
- run_id=dagrun_id,
+ run_id=dag_run_id,
)
Review comment:
yeah you are right
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815219255
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
i understand, but i disagree.
in my experience, if you have an order_line table, you call the identity column either `order_line_id` or simply `id`, but not `orderline_id`. you don't violate your snake case convention for identity columns.
and there's good reason we use snake case. consider table `ab_permission_view_role`. would you really want to call that table's identity column `abpermissionviewrole_id`?
actually, looking at that table, its ident col is `id` -- maybe we should just go with that here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r816306151
##########
File path: airflow/models/xcom.py
##########
@@ -160,26 +160,26 @@ def set(
message = "Passing 'execution_date' to 'XCom.set()' is deprecated. Use 'run_id' instead."
warnings.warn(message, DeprecationWarning, stacklevel=3)
try:
- dagrun_id, run_id = (
+ dag_run_id, run_id = (
session.query(DagRun.id, DagRun.run_id)
.filter(DagRun.dag_id == dag_id, DagRun.execution_date == execution_date)
.one()
)
except NoResultFound:
raise ValueError(f"DAG run not found on DAG {dag_id!r} at {execution_date}") from None
- elif run_id == IN_MEMORY_DAGRUN_ID:
- dagrun_id = -1
+ elif run_id == IN_MEMORY_RUN_ID:
+ dag_run_id = -1
else:
- dagrun_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
- if dagrun_id is None:
+ dag_run_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
+ if dag_run_id is None:
raise ValueError(f"DAG run not found on DAG {dag_id!r} with ID {run_id!r}")
value = cls.serialize_value(
value=value,
key=key,
task_id=task_id,
dag_id=dag_id,
- run_id=dagrun_id,
+ run_id=dag_run_id,
)
Review comment:
yeah you are right, i'll fix
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#issuecomment-1051703737
side note, question for @uranusjr :
this bit here: https://github.com/apache/airflow/pull/21806/files/f3aaa031be561d3e6671dbd734bb427fe283ddf8#diff-3f95f161bd9ef4bb455611e0c58583899769360afc53f755cd1577cf194553c5R70-R79
now that we are keying xcom to the PK of dag_run, we should be able to look up the dag run by the PK rather than doing this join:
```python
primaryjoin="""and_(
BaseXCom.dag_id == foreign(DagRun.dag_id),
BaseXCom.run_id == foreign(DagRun.run_id),
)""",
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815339573
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
> And dag_run is also an outlier in Airflow since most other tables e.g. taskinstance do lack the undercores).
it's not actually true, they are all snake case. the table for `TaskInstance` is `task_instance`. and there's not a single table in airflow db that's not snake case table name. (But the _python module_ names are like that.)
> I guess this is essentially down to personally preference
To some extent that's true but consistency and convention also count for something. Particularly in the context of database object naming conventions, in this area it's a lot easier to make conventions about than some other decisions we have to make. And referring to the PK as `f"{table_name}_id` is i think pretty standard[1]. E.g. you would not create a table like this:
```sql
create table dag_run (
dagrun_id int identity,
val varchar
)
```
That would not make any sense. Why not call the _table_ `dagrun` in that case? I think it's essentially the same here. The table is called `dag_run` so it's ID should be referred to as `dag_run_id`. Since the table names have already made the decision to go snake case, why should the columns go backwards on that decision? And it's not that we can't ever deviate from the standard choice, but that side of the argument bears the burden; there should be a good reason to make an exception.
As always, I'll go with the majority view, but just trying to present what makes sense to me. And of course it's always better if we can reach consensus.
---
[1] Jetbrains can even [suggest joins](https://www.jetbrains.com/help/datagrip/foreign-keys.html#configure-rules-for-virtual-foreign-keys) for this table naming pattern when no FK is defined.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r814495413
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
IMO `dagrun_id` is more appropriate here because it refers to the `id` column on the `dagrun` table.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815339573
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
> And dag_run is also an outlier in Airflow since most other tables e.g. taskinstance do lack the undercores).
it's not actually true, they are all snake case. the table for `TaskInstance` is `task_instance`. and there's not a single table in airflow db that's not snake case table name. (But the _python module_ names are like that.)
> I guess this is essentially down to personally preference
To some extent that's true but consistency and convention also count for something. Particularly in the context of database object naming conventions, in this area it's a lot easier to make conventions about than some other decisions we have to make. And referring to the PK as `f"{table_name}_id` is i think pretty standard[1]. E.g. you would not create a table like this:
```sql
create table dag_run (
dagrun_id int identity,
val varchar
)
```
That would not make any sense. Why not call the table `dagrun` in that case? I think it's essentially the same here. The table is called `dag_run` so it's ID should be referred to as `dag_run_id`. Since the table names have already made the decision to go snake case, why should the columns go backwards on that decision? And it's not that we can't ever deviate from the standard choice, but that side of the argument bears the burden; there should be a good reason to make an exception.
As always, I'll go with the majority view, but just trying to present what makes sense to me. And of course it's always better if we can reach consensus.
---
[1] Jetbrains can even [suggest joins](https://www.jetbrains.com/help/datagrip/foreign-keys.html#configure-rules-for-virtual-foreign-keys) for this table naming pattern when no FK is defined.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r814535864
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
does that change your feeling about this?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#issuecomment-1050306686
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r816304219
##########
File path: airflow/models/xcom.py
##########
@@ -160,26 +160,26 @@ def set(
message = "Passing 'execution_date' to 'XCom.set()' is deprecated. Use 'run_id' instead."
warnings.warn(message, DeprecationWarning, stacklevel=3)
try:
- dagrun_id, run_id = (
+ dag_run_id, run_id = (
session.query(DagRun.id, DagRun.run_id)
.filter(DagRun.dag_id == dag_id, DagRun.execution_date == execution_date)
.one()
)
except NoResultFound:
raise ValueError(f"DAG run not found on DAG {dag_id!r} at {execution_date}") from None
- elif run_id == IN_MEMORY_DAGRUN_ID:
- dagrun_id = -1
+ elif run_id == IN_MEMORY_RUN_ID:
+ dag_run_id = -1
else:
- dagrun_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
- if dagrun_id is None:
+ dag_run_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
+ if dag_run_id is None:
raise ValueError(f"DAG run not found on DAG {dag_id!r} with ID {run_id!r}")
value = cls.serialize_value(
value=value,
key=key,
task_id=task_id,
dag_id=dag_id,
- run_id=dagrun_id,
+ run_id=dag_run_id,
)
Review comment:
Hmm, is this a bug? Passing in the run’s primary key seems wrong.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#issuecomment-1050616716
@ashb re
> I think this is fine. Any user/developer-facing backcompat concerns?
I think we're ok here.
i think the only changes that could possibly cause an issue are these:
airflow.api.common.mark_tasks.set_state (signature change, rename param `dag_run_id` to `run_id`)
IN_MEMORY_RUN_ID (rename from IN_MEMORY_DAG_RUN_ID)
DagRunNotBackfillDep (rename from DagrunIdDep)
I think all of these can reasonably be considered internal.
And as mentioned in the description above, there will remain API parameters `dag_run_id` which will refer to the dag run run_id. But I think that's OK and sortof orthogonal to the addition of this dag run ID column. If we ever need to expose the integer Id, we'd probably want to rename "dag_run_id" to "run_id" anyway, rather than having two params "dag_run_id" and "dagrun_id" coexisting together.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815333294
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
I guess this is essentially down to personally preference. I won’t insist if you feel `dag_run_id` is better.
(Personally I never use underscores in my table names, so this is kind of undefined in my mental style guide. And `dag_run` is also an outlier in Airflow since most other tables e.g. `taskinstance` do lack the undercores).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#issuecomment-1052331918
> now that we are keying xcom to the PK of dag_run, we should be able to look up the dag run by the PK rather than doing this join:
Good point, I think I simply missed changing this when adding the `dagrun_id` fk.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r815261308
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
Ah i see we're talking about the column in the xcom table, which is pointing to the dag run table.
The PK in dag_run is already `id`.
So yeah, same reasoning essentially applies. When we refer to id col of a table, it should be `f"{table_name}_{col_name}"`. I don't think there's really any reason to diverge from this convention here. There's no "run_id" on the "dag" table so there's no conflict here.
Can find precedent for this with column `view_menu_id` in table `ab_permission_view` (which keys to `ab_view_menu.id`), and column `permission_view_id` in table `ab_permission_view_role` (which keys to `ab_permission_view.id`).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #21806: Rename `dagrun_id` to `dag_run_id`
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #21806:
URL: https://github.com/apache/airflow/pull/21806#discussion_r814505539
##########
File path: airflow/migrations/versions/c306b5b5ae4a_switch_xcom_table_to_use_run_id.py
##########
@@ -41,7 +41,7 @@
def _get_new_xcom_columns() -> Sequence[Column]:
return [
- Column("dagrun_id", Integer(), nullable=False),
+ Column("dag_run_id", Integer(), nullable=False),
Review comment:
the table is actually `dag_run` -- not `dagrun`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org