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

Issue 9748, views are working after my tests #9802

Closed
wants to merge 3 commits into from

Conversation

johanteekens
Copy link

@johanteekens johanteekens commented Jan 2, 2024

Description

current situation: SQLDatabase(engine, include_tables=["city_stats"])
obj_index = ObjectIndex.from_objects(table_schema_objs,table_node_mapping,VectorStoreIndex,)
query_engine = SQLTableRetrieverQueryEngine(sql_database, obj_index.as_retriever(),)

preferred new option: SQLDatabase(engine, include_databaseobjects=["city_stats","myview1","mytable1","myview2"])

Fixes # It seems views was already implemented but not enabled. After enabling I ran soms tests and it seems to work for me.

Type of Change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
Screenshot 2024-01-02 at 21 49 28

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jan 2, 2024
custom_table_info: Optional[dict] = None,
view_support: bool = False,
view_support: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some consequence for changing the default vs. Letting the dev make the decision?

Does this still work fine for normal tables without views?

Copy link
Author

Choose a reason for hiding this comment

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

For me it does work with tables and views. I also tried a mix with tables and views. it's not a big change but I'd rather have someone else test this as well before me breaking the whole framework.

@logan-markewich
Copy link
Collaborator

Since the option exists, I'd rather not change the defaults for now I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants