feat: support SparkJDBCDataset JDBC URLs in credentials - #1462
Conversation
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
Kedro PR Review
Generated with
review-kedro-prskill.
Overview
- Critical: 1 | Suggestions: 3
Thanks @Shizoqua — this aligns SparkJDBCDataset with how SQLTableDataset handles connection strings via credentials, and the implementation looks solid (explicit url precedence, no credentials mutation, url excluded from JDBC properties).
Critical (must fix before merge)
- CI —
detect-secrets/merge-gatekeeperare failing. The hook reports that.secrets.baselineneeds updating after the docstring changes. Please run the repo's detect-secrets hook (ordetect-secrets scan --update .secrets.baseline) and commit the baseline update.
Suggestions (consider improving)
- Add @Shizoqua under
Community contributionsinRELEASE.md. - Add a symmetric
load()precedence test to matchtest_save_explicit_url_takes_precedence_over_credentials_url. - Optional: cross-reference the
SQLTableDatasetcredentials.conpattern in the docstring.
Notes
- General review: Focused PR, good tests, backward compatible for existing catalogs with explicit
url. - PR checklist: Docs and RELEASE updated; tests added; DCO signed.
- API usage: Correct
AbstractDatasetusage; no breaking changes for current callers.
|
Hi @Shizoqua , Thanks for the contribution. The code looks good to me. Left minor comments and a required CI fix to go in before we merge this. Otherwise looks nice !! 💯 |
…ence test, docstring note Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
e5a7b18 to
7d341e0
Compare
Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
|
thanks @ravi-kumar-pilla i have addressed Suggestions and fixed CI failure |
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
LGTM...thank you @Shizoqua
Signed-off-by: L. R. Couto <57910428+lrcouto@users.noreply.github.com>
|
Thanks for the PR @Shizoqua ! One small thing I'd like to point out is that the database URL passed in the credentials is being exposed by the db_credentials = {
"url": "jdbc:postgresql://prod-db.internal:5432/table_name",
"user": "johndoe",
"password": "super-safe-password",
}
dataset = SparkJDBCDataset(table="table_name", credentials=db_credentials)
described = dataset._describe()
print(described)If you're using pandas.SQLTableDataset as a reference, I think you could just drop the url key from Other than this, I think it looks good! |
Signed-off-by: shizoqua <hr.lanreshittu@yahoo.com>
b5d885e to
cf5958a
Compare
|
Thanks for catching that! I’ve updated |
Description
Closes #1286.
This PR allows
spark.SparkJDBCDatasetto usecredentials.urlas the JDBC URL when the expliciturlargument is not provided.Development notes
credentials["url"].urlprecedence over credentials.urlfrom the Spark JDBCpropertiespassed to load/save.kedro-datasets/RELEASE.md.Checked locally:
python -m ruff check kedro_datasets/spark/spark_jdbc_dataset.py tests/spark/test_spark_jdbc_dataset.pygit diff --checkpython -m compileall kedro_datasets/spark/spark_jdbc_dataset.py tests/spark/test_spark_jdbc_dataset.pyAttempted locally:
python -m pytest tests/spark/test_spark_jdbc_dataset.pydid not run because the local environment is missingaiobotocore, which is imported bytests/conftest.py.Developer Certificate of Origin
The commit includes a
Signed-off-byline.Checklist
jsonschema/kedro-catalog-X.XX.jsonif necessary (not necessary)RELEASE.mdfile