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

.Net: Fix KustoMemoryStore reading Timestamp column data type #5600

Merged
merged 16 commits into from
Apr 16, 2024

Conversation

mohammedtabish0
Copy link
Contributor

@mohammedtabish0 mohammedtabish0 commented Mar 21, 2024

Fixes: #5599

Description

While reading data from Kusto proper data type should be used in KustoMemoryStore.

@mohammedtabish0 mohammedtabish0 requested a review from a team as a code owner March 21, 2024 16:10
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory labels Mar 21, 2024
@github-actions github-actions bot changed the title fixes issue 5599 .Net: fixes issue 5599 Mar 21, 2024
@markwallace-microsoft markwallace-microsoft self-assigned this Mar 22, 2024
Copy link
Member

@markwallace-microsoft markwallace-microsoft left a comment

Choose a reason for hiding this comment

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

Just one comment on the code duplication

Copy link
Member

@markwallace-microsoft markwallace-microsoft left a comment

Choose a reason for hiding this comment

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

There is a failing unit test that needs to be fixed

@mohammedtabish0
Copy link
Contributor Author

mohammedtabish0 commented Mar 22, 2024

Just one comment on the code duplication

There was a mistake we won't get similarity in GetBatchAsync() method so removed it and hence cannot club the same logic in a method. It is possible but would look ugly and complex imo.

@mohammedtabish0
Copy link
Contributor Author

There is a failing unit test that needs to be fixed

Updated test cases

@markwallace-microsoft markwallace-microsoft changed the title .Net: fixes issue 5599 .Net: Fix KustoMemoryStore reading Timestamp column data type Mar 22, 2024
@markwallace-microsoft
Copy link
Member

@mohammedtabish0 can you cd into the dotnet folder and run dotnet format

@mohammedtabish0
Copy link
Contributor Author

@mohammedtabish0 can you cd into the dotnet folder and run dotnet format

My bad forgot to run after the changes.

@mohammedtabish0
Copy link
Contributor Author

Updated the code, on local dotnet build didn't gave it as an error but as a warning. Updated the code please review again.

@mohammedtabish0
Copy link
Contributor Author

@markwallace-microsoft can we merge?

@markwallace-microsoft
Copy link
Member

@markwallace-microsoft can we merge?

@mohammedtabish0 Can you look at the comments above and once these are resolved I'll get this merged

@mohammedtabish0
Copy link
Contributor Author

@markwallace-microsoft can we merge?

@mohammedtabish0 Can you look at the comments above and once these are resolved I'll get this merged

Updated based on comments

@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Apr 16, 2024
Merged via the queue into microsoft:main with commit 67233e5 Apr 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel memory .NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

KustoMemoryStore reading Timestamp column data type
6 participants