Skip to content

Use eval_result/metadata for comparision in case of dml/ddl#89

Merged
hardikgu23 merged 3 commits intomainfrom
seperateExecution
Nov 14, 2024
Merged

Use eval_result/metadata for comparision in case of dml/ddl#89
hardikgu23 merged 3 commits intomainfrom
seperateExecution

Conversation

@hardikgu23
Copy link
Copy Markdown
Collaborator

@hardikgu23 hardikgu23 commented Nov 11, 2024

We are currently storing eval_query output for dml queries and complete schema metadata for ddl (#87) . Using the execution result of these to compare if generated query was correct or not (as dml/ddl execution do not return any result on their execution)

@IsmailMehdi
Copy link
Copy Markdown
Collaborator

_ No description provided. _

Please have descriptions in you PRs.

IsmailMehdi
IsmailMehdi previously approved these changes Nov 11, 2024
@hardikgu23
Copy link
Copy Markdown
Collaborator Author

Added description.

Copy link
Copy Markdown
Collaborator

@mahyareb mahyareb left a comment

Choose a reason for hiding this comment

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

@hardikgu23 instead of overriding generated and golden result, could you just pass eval_results and golden_eval_results to scorer in comp.compare

And as of right now, we only should use exact_match scorer for DML / DDL. So we could add a flag in the config like

...
scorers:
  exact_match:
     use_eval_sql: true
...

@IsmailMehdi what do you think?

@IsmailMehdi
Copy link
Copy Markdown
Collaborator

@hardikgu23 instead of overriding generated and golden result, could you just pass eval_results and golden_eval_results to scorer in comp.compare

And as of right now, we only should use exact_match scorer for DML / DDL. So we could add a flag in the config like

...
scorers:
  exact_match:
     use_eval_sql: true
...

@IsmailMehdi what do you think?

Yes that sounds reasonable

@hardikgu23
Copy link
Copy Markdown
Collaborator Author

hardikgu23 commented Nov 13, 2024

@hardikgu23 instead of overriding generated and golden result, could you just pass eval_results and golden_eval_results to scorer in comp.compare

And as of right now, we only should use exact_match scorer for DML / DDL. So we could add a flag in the config like

...
scorers:
  exact_match:
     use_eval_sql: true
...

@IsmailMehdi what do you think?

Acknowledged. Have modified the pr to do the same.
Here is the sample run id: cbb68549-323b-4669-b588-bdf084e6d186

mahyareb
mahyareb previously approved these changes Nov 14, 2024
Copy link
Copy Markdown
Collaborator

@mahyareb mahyareb left a comment

Choose a reason for hiding this comment

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

LGTM! Just left a comment for one minor thing to fix. Thanks @hardikgu23 ! 👏

@hardikgu23 hardikgu23 merged commit d130406 into main Nov 14, 2024
@hardikgu23 hardikgu23 deleted the seperateExecution branch November 14, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants