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 2021/04/29 04:07:13 UTC
[GitHub] [airflow] suiting-young opened a new pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
suiting-young opened a new pull request #15585:
URL: https://github.com/apache/airflow/pull/15585
`sqlalchemy.func.sum` has a known *"issue"* that it **may** returns `Decimal` value (_in my case MySQL 5.7_).
This will cause problem when calling [rest APIs](https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#tag/Pool). E.g.
```http
GET /airflow/api/v1/pools?limit=100
...
TypeError: Object of type 'Decimal' is not JSON serializable
```
@ashb and @kaxil please have a review, thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] suiting-young commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
suiting-young commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622745595
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,12 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int((
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ ) or 0)
Review comment:
@uranusjr I tried this first, but failed, when filter matched nothing, it returns `None` ðŸ˜
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] suiting-young commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
suiting-young commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622746652
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,12 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int((
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ ) or 0)
Review comment:
Per [MySQL doc](https://dev.mysql.com/doc/refman/8.0/en/aggregate-functions.html#function_sum):
> If there are no matching rows, SUM() returns NULL.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622720825
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,12 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int((
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ ) or 0)
Review comment:
`0` does not need to be converted, so I’d just cast the query part:
```python
return int(
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
) or 0
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #15585:
URL: https://github.com/apache/airflow/pull/15585
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#issuecomment-828966467
Hi @suiting-young , the change makes sense to me. Please fix the static check errors. Thanks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622996974
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,13 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int(
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ or 0
Review comment:
This is the implementation, there's no magic at all. I guess the SO answer is inaccurate.
https://github.com/sqlalchemy/sqlalchemy/blob/b703a9c1be728077e4709c377b0b29a94c052a0b/lib/sqlalchemy/orm/query.py#L2818-L2824
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622746897
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,12 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int((
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ ) or 0)
Review comment:
Oh… alright then.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15585: `func.sum` may returns `Decimal` that break rest APIs.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15585:
URL: https://github.com/apache/airflow/pull/15585#discussion_r622980931
##########
File path: airflow/models/pool.py
##########
@@ -161,12 +161,13 @@ def occupied_slots(self, session: Session):
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
- return (
+ return int(
session.query(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
.filter(TaskInstance.state.in_(list(EXECUTION_STATES)))
.scalar()
- ) or 0
+ or 0
Review comment:
Interesting I thought `.scalar()` should take care of it based on https://stackoverflow.com/a/53431701/5691525 too
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org