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/09/25 15:01:12 UTC
[GitHub] [airflow] alexandermalyga opened a new pull request, #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
alexandermalyga opened a new pull request, #26661:
URL: https://github.com/apache/airflow/pull/26661
Added ON CONFLICT DO NOTHING statement when `replace=True` and all target fields are primary keys.
Also made `_generate_insert_sql()` use `cls.placeholder` for standardization sake.
Previously, if all inserted columns were primary keys, the generated SQL statement would not be syntactically correct.
--
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 diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r979497653
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
It’s slightly weird to me that `replace=True` (without setting `replace_index`) triggers `ON CONFLICT DO NOTHING` 🤔
--
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] kaxil commented on pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
kaxil commented on PR #26661:
URL: https://github.com/apache/airflow/pull/26661#issuecomment-1260057194
cc @utkarsharma2
--
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 diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r979497653
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
It’s slightly weird to me that `replace=True, replace_index=()` triggers `ON CONFLICT DO NOTHING` 🤔 We should probably list out all possible argument combinations and try to figure out which behaviour is best for each.
--
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] alexandermalyga commented on pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on PR #26661:
URL: https://github.com/apache/airflow/pull/26661#issuecomment-1305776543
@potiuk @eladkal could you have a look please?
--
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] alexandermalyga commented on a diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r980346489
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
All possible combinations are, assuming `replace=True`:
1. `target_fields=(), replace_index=()`: any of these being empty or None should raise an exception
2. `target_fields=('a', 'b'), replace_index=('a')`: should trigger `ON CONFLICT DO UPDATE SET b = excluded.b`
3. `target_fields=('a', 'b'), replace_index=('a', 'b')`: should trigger `ON CONFLICT DO NOTHING`
4. `target_fields=('a'), replace_index=('a', 'b')`: will raise an exception because a mandatory field is not provided, or not conflict because the value was autogenerated by the DB
All of these behaviours are currently implemented, besides the check for an empty list which I'll implement and push now.
Can't think of any other situation, please correct me if I'm 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] alexandermalyga commented on a diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r980346489
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
All possible combinations are, assuming `replace=True`:
1. `target_fields=(), replace_index=()`: any of these being empty or None should raise an exception.
2. `target_fields=('a', 'b'), replace_index=('a')`: should trigger `ON CONFLICT DO UPDATE SET b = excluded.b`.
3. `target_fields=('a', 'b'), replace_index=('a', 'b')`: should trigger `ON CONFLICT DO NOTHING`.
4. `target_fields=('a'), replace_index=('a', 'b')`: will raise an exception because a mandatory field is not provided, or not conflict because the value was autogenerated by the DB.
All of these behaviours are currently implemented, besides the check for an empty list which I'll implement and push now.
Can't think of any other situation, please correct me if I'm 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] alexandermalyga commented on a diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r980346489
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
All possible combinations are, assuming `replace=True`:
1. `target_fields=(), replace_index=()`: any of these being empty or None should raise an exception
2. `target_fields=('a', 'b'), replace_index=('a')`: should trigger `ON CONFLICT DO UPDATE SET b = excluded.b`
3. `target_fields=('a', 'b'), replace_index=('a', 'b')`: should trigger `ON CONFLICT DO NOTHING`
4. `target_fields=('a'), replace_index=('a', 'b')`: will raise an SQL exception because a mandatory field is not provided, or not conflict because the value was auto-generated by the DB
All of these behaviours are currently implemented, besides the check for an empty list which I'll implement and push now.
Can't think of any other situation, please correct me if I'm 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] potiuk merged pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26661:
URL: https://github.com/apache/airflow/pull/26661
--
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] alexandermalyga commented on a diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r980335960
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
It will not trigger `ON CONFLICT DO NOTHING` because the check is done against `replace_target` and not `replace_index`.
But now that I think about it, if `replace_index=()`, the generated SQL will not be valid: `ON CONFLICT ()`. To avoid that we should change `if replace_index is None:` to `if not replace_index:` before raising the exception.
--
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] alexandermalyga commented on a diff in pull request #26661: PostgresHook: Added ON CONFLICT DO NOTHING statement when all target fields are primary keys
Posted by GitBox <gi...@apache.org>.
alexandermalyga commented on code in PR #26661:
URL: https://github.com/apache/airflow/pull/26661#discussion_r1001014577
##########
airflow/providers/postgres/hooks/postgres.py:
##########
@@ -270,10 +270,14 @@ def _generate_insert_sql(
raise ValueError("PostgreSQL ON CONFLICT upsert syntax requires an unique index")
if isinstance(replace_index, str):
replace_index = [replace_index]
- replace_index_set = set(replace_index)
- replace_target = [
- "{0} = excluded.{0}".format(col) for col in target_fields if col not in replace_index_set
- ]
- sql += f" ON CONFLICT ({', '.join(replace_index)}) DO UPDATE SET {', '.join(replace_target)}"
+ on_conflict_str = f" ON CONFLICT ({', '.join(replace_index)})"
+ replace_target = [f for f in target_fields if f not in replace_index]
+
+ if replace_target:
+ replace_target_str = ", ".join(f"{col} = excluded.{col}" for col in replace_target)
+ sql += f"{on_conflict_str} DO UPDATE SET {replace_target_str}"
+ else:
+ sql += f"{on_conflict_str} DO NOTHING"
Review Comment:
Hey @uranusjr did you have a chance to have a look? Could this be merged?
--
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