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/04/18 15:31:39 UTC
[GitHub] [airflow] dstandish opened a new pull request, #23056: Add server default for map_index in Log table
dstandish opened a new pull request, #23056:
URL: https://github.com/apache/airflow/pull/23056
When logging CLI actions we insert a record into the Log table. But for 2.3 we add column map_index to Log, and if the Log model expects map_index to be there the insert will fail and a warning will be emitted.
We can avoid the error and warning by adding a server_default of NULL on map_index in Log. I choose NULL instead of -1 because
generally speaking map_index doesn't make sense for Log tables.
--
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 #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #23056:
URL: https://github.com/apache/airflow/pull/23056#issuecomment-1102675192
> From what I understand, `server_default` is injected into `ALTER TABLE` when the table is being modified (or `CREATE TABLE`), and without adding this to a migration this would do nothing.
So it does _something_ -- namely, it seems to make it so that when it generates an insert, it doesn't include `map_index` as a column when it's not set. I learned of the behavior [here](https://stackoverflow.com/questions/6536600/sqlalchemy-omit-attribute-in-insert-column-list/). And I did test it locally. I don't think adding to migration would change the behavior (since NULL is already the server default, right?) but it's easy enough to do so I'll add that in a sec.
> Instead of `server_default` perhaps we need to add `self.map_index = None` in `__init__` instead.
I just tried this and it doesn't work. I suspect it already gets a None value since it doesn't get set in init.
You can test locally on main by taking a fresh db, dropping map_index from log table, and running `db upgrade`.
--
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 #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #23056:
URL: https://github.com/apache/airflow/pull/23056
--
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 #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #23056:
URL: https://github.com/apache/airflow/pull/23056#issuecomment-1101535601
I think this needs a change in the migration file as well?
--
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 #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #23056:
URL: https://github.com/apache/airflow/pull/23056#issuecomment-1101561056
> I think this needs a change in the migration file as well?
@uranusjr in the migration file it's nullable which means essentially that the server default is NULL already. So I don't think we need to change anything more but let me know if you think there's something we need to do.
--
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 #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23056:
URL: https://github.com/apache/airflow/pull/23056#issuecomment-1101514377
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 pull request #23056: Add server default for map_index in Log table
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #23056:
URL: https://github.com/apache/airflow/pull/23056#issuecomment-1101862748
> in the migration file it's nullable which means essentially that the server default is NULL already.
But the definition in the class also declares nullable (in the sense that `nullable` is not False and True is the default). From what I understand, `server_default` is injected into `ALTER TABLE` when the table is being modified (or `CREATE TABLE`), and without adding this to a migration this would do nothing.
Instead of `server_default` perhaps we need to add `self.map_index = None` in `__init__` instead.
--
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