Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Buggy SQL for postgres source #3424

Merged
merged 2 commits into from
Jan 15, 2023
Merged

Conversation

xaniasd
Copy link
Contributor

@xaniasd xaniasd commented Dec 30, 2022

What this PR does / why we need it:

Removes parentheses that lead to SQL syntax error when calling get_table_column_names_and_types

Which issue(s) this PR fixes:

No corresponding issue created.

@xaniasd xaniasd changed the title fix: get_table_column_names_and_types for postgres source fix: buggy SQL for postgres source Dec 30, 2022
@xaniasd xaniasd changed the title fix: buggy SQL for postgres source Fix: buggy SQL for postgres source Dec 30, 2022
@xaniasd xaniasd changed the title Fix: buggy SQL for postgres source fix: Buggy SQL for postgres source Dec 30, 2022
@achals
Copy link
Member

achals commented Jan 2, 2023

@xaniasd do you have a stack trace of the error that manifests with the parentheses?

@achals
Copy link
Member

achals commented Jan 2, 2023

/ok-to-test

@xaniasd
Copy link
Contributor Author

xaniasd commented Jan 3, 2023

hi @achals
in order to reproduce the bug, you need a PostgreSQLSource specifying a table instead of a query. Then the error shows up e.g. with feast plan. Using feast 0.27.1 btw. Hope this helps!

Traceback (most recent call last):
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/bin/feast", line 8, in <module>
    sys.exit(cli())
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/cli.py", line 497, in plan_command
    plan(repo_config, repo, skip_source_validation)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/usage.py", line 288, in wrapper
    return func(*args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/repo_operations.py", line 217, in plan
    registry_diff, infra_diff, _ = store.plan(repo)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/usage.py", line 299, in wrapper
    raise exc.with_traceback(traceback)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/usage.py", line 288, in wrapper
    return func(*args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/feature_store.py", line 724, in plan
    self._make_inferences(
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/feature_store.py", line 602, in _make_inferences
    update_feature_views_with_inferred_features_and_entities(
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/inference.py", line 168, in update_feature_views_with_inferred_features_and_entities
    _infer_features_and_entities(
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/inference.py", line 206, in _infer_features_and_entities
    table_column_names_and_types = fv.batch_source.get_table_column_names_and_types(
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/typeguard/__init__.py", line 1033, in wrapper
    retval = func(*args, **kwargs)
  File "/Users/dimitris.stafylarakis/Library/Caches/pypoetry/virtualenvs/feast-test-repo-iTsfB37N-py3.9/lib/python3.9/site-packages/feast/infra/offline_stores/contrib/postgres_offline_store/postgres_source.py", line 114, in get_table_column_names_and_types
    cur.execute(
psycopg2.errors.SyntaxError: syntax error at or near ")"
LINE 1: SELECT * FROM (driver_hourly_stats) AS sub LIMIT 0
                                          ^                                     

Copy link
Collaborator

@adchia adchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, xaniasd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xaniasd
Copy link
Contributor Author

xaniasd commented Jan 10, 2023

hi @adchia some integration tests failed after your last approval (unrelated to the code change as far as I could tell) so I forced them to run again. I'm not really familiar with the build system but they seem to be ok now, can you approve again please?

@adchia
Copy link
Collaborator

adchia commented Jan 12, 2023

/lgtm

@xaniasd
Copy link
Contributor Author

xaniasd commented Jan 12, 2023

@adchia thanks, still failing on the go linting check. What do you suggest?

@adchia
Copy link
Collaborator

adchia commented Jan 12, 2023

i believe this is a build breakage that @felixwang9817 is investigating

@xaniasd
Copy link
Contributor Author

xaniasd commented Jan 12, 2023

ok I'll wait for the fix then 👍

@felixwang9817
Copy link
Collaborator

/lgtm

@felixwang9817 felixwang9817 merged commit 1ea100e into feast-dev:master Jan 15, 2023
kevjumba pushed a commit that referenced this pull request Jan 31, 2023
# [0.29.0](v0.28.0...v0.29.0) (2023-01-31)

### Bug Fixes

* Add check for bool type in addition to sample ([#3452](#3452)) ([1c7c491](1c7c491))
* Buggy SQL for postgres source ([#3424](#3424)) ([1ea100e](1ea100e))
* Ensure no duplicates in `fv.schema` ([#3460](#3460)) ([08ffa8d](08ffa8d))
* Fix delete sfv twice issue ([#3466](#3466)) ([dfd5eae](dfd5eae))
* Stream feature view UI shows transformation issue ([#3464](#3464)) ([1ef5137](1ef5137))
* Update registry.refresh to have a default arg ([#3450](#3450)) ([2f7c4ed](2f7c4ed))
* Updating the batch field so that you can query create and event date. ([#3411](#3411)) ([01ab462](01ab462)), closes [#3401](#3401)

### Features

* Add data source search ([#3449](#3449)) ([fbbb293](fbbb293))
* Adding list_validation_references for default and sql registry ([#3436](#3436)) ([21dd253](21dd253))
* Make UI accessible behind proxy ([#3428](#3428)) ([753d8db](753d8db))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants