Skip to content
This repository was archived by the owner on May 13, 2025. It is now read-only.

Conversation

@Koustavd18
Copy link
Contributor

@Koustavd18 Koustavd18 commented Sep 12, 2024

Support for multiple query engine

Changes

  • new class implementation which generates new query and resource path based on the query engine
  • new filter query class which generates new filter query
  • SQL query based on based on query engine

@Koustavd18 Koustavd18 marked this pull request as draft September 13, 2024 15:01
@Koustavd18 Koustavd18 self-assigned this Sep 13, 2024
@Koustavd18 Koustavd18 marked this pull request as ready for review September 14, 2024 09:43
src/api/query.ts Outdated
Copy link
Contributor

@nativ-labs nativ-labs Sep 14, 2024

Choose a reason for hiding this comment

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

Why'd we did a class implementation ? The created instance was never passed outside this function. So there is no need for a class here.

Give a read on Separation of concern.

Copy link
Member

Choose a reason for hiding this comment

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

Problem we want to solve is that earlier there was this useTrino flag passed around everywhere which is error prone. How do you propose to fix that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok then.

Copy link
Member

Choose a reason for hiding this comment

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

I am more than happy if you can propose a better / idiomatic approach on how to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Even now, we are sending the 'queryEngine' in place of 'useTrino'. So no difference.
  2. App initializes only one time. We should assume the query engine the first time itself and save it as a boolean (useTrino: boolean, since we are not going to include any other connector anytime soon) in the AppProvider. So we dont have to compare the string everytime, everywhere.
  3. trinoquery & query endpoint definition should remain separate.

Stating all the above, we dont really need a class. Instead of passing the string everywhere, pass just the boolean since thats what we need at the end. Revert back all the changes and just set and set the useTrino (boolean) in the app provider.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing the string everywhere, pass just the boolean since thats what we need at the end.

We have to avoid this. It has to dynamically decide which engine to use based on API response.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats a very valid point if we are going to add more connectors.

Copy link
Member

Choose a reason for hiding this comment

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

Right now there is one, but we have to make it modular so we can extend as needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about the endpoint def. 1 function to dynamically decide or separate endpoints for every connectors?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to include more connectors:

Then the class should provide everything for making a request.
All the logic related to query engine should be inside the class. Define separate urls for separate engines and the class should decide and return the url also.

Copy link
Member

Choose a reason for hiding this comment

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

Why remove LIMIT OFFSET in normal query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this for the count query

@nitisht nitisht merged commit 2d96239 into parseablehq:main Sep 17, 2024
1 of 3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2024
@Koustavd18 Koustavd18 deleted the queryEngine branch September 30, 2024 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants