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

Refactor value converters and implement ExtractValues for Row #75

Merged
merged 4 commits into from
Feb 21, 2022
Merged

Refactor value converters and implement ExtractValues for Row #75

merged 4 commits into from
Feb 21, 2022

Conversation

w1ndy
Copy link
Contributor

@w1ndy w1ndy commented Feb 9, 2022

Pull Request Description

This PR proposes a fix for #72 , which includes the following changes:

  1. The logic of boolConvert, dateTimeConvert, etc are decoupled from Row.ToStruct. Now, each value type has its own Convert method, which transforms its value into a reflect.Value. All Convert methods are taken as is from the logic used by Row.ToStruct. Moreover, each value type is placed in its own go source file because the value.go file has too many lines.
  2. A new method called ExtractValues is added to struct Row. This method implements the intended functionality of Columns, which allows users to unpack the row's values into interfaces, enabling convenient and simplified access to these values.
  3. Fix the docs of Row.Columns.

ExtractValues can be used like this:

var userId int64
var userName string
row.ExtractValues(&userId, &userName)

Future Release Comment

Features:

  • Row.ExtractValues is added to easily unpack a row's values into a set of interfaces.

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2022

Codecov Report

Merging #75 (76836fb) into master (9a620e9) will decrease coverage by 5.28%.
The diff coverage is 40.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   52.42%   47.13%   -5.29%     
==========================================
  Files          30       39       +9     
  Lines        4273     4226      -47     
==========================================
- Hits         2240     1992     -248     
- Misses       1871     2092     +221     
+ Partials      162      142      -20     
Impacted Files Coverage Δ
kusto/data/value/dynamic.go 19.00% <19.00%> (ø)
kusto/data/value/decimal.go 24.44% <24.44%> (ø)
kusto/data/value/bool.go 28.88% <28.88%> (ø)
kusto/data/value/string.go 32.50% <32.50%> (ø)
kusto/data/value/datetime.go 36.00% <36.00%> (ø)
kusto/data/value/guid.go 38.63% <38.63%> (ø)
kusto/data/value/real.go 38.77% <38.77%> (ø)
kusto/data/value/long.go 40.00% <40.00%> (ø)
kusto/data/value/int.go 43.85% <43.85%> (ø)
kusto/data/table/table.go 34.24% <45.45%> (+1.98%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3af6cd...76836fb. Read the comment docs.

@AsafMah
Copy link
Contributor

AsafMah commented Feb 14, 2022

It seems like that in the refactoring we lost this logic before each conversion (for each type it's slightly different of course):

val, ok := k.(value.Bool)
	if !ok {
		return fmt.Errorf("Column %s is type %s was trying to store a KustoValue type of %T", col.Name, col.Type, k)
	}

Is there a reason for removing this?

Are there more changes to note?

@w1ndy
Copy link
Contributor Author

w1ndy commented Feb 15, 2022

It seems like that in the refactoring we lost this logic before each conversion (for each type it's slightly different of course):

val, ok := k.(value.Bool)
	if !ok {
		return fmt.Errorf("Column %s is type %s was trying to store a KustoValue type of %T", col.Name, col.Type, k)
	}

Is there a reason for removing this?

Are there more changes to note?

To my best understanding, such a type conversion is not needed if we have a polymorphism method Convert on the interface value.Kusto. In the prior implementation, each convert method accepts different types of value.Kusto, so we'll have to manually do type casting before passing the values into these methods. Now, we can call the convert method on value.Kusto directly, and the implementation that will be executed depends on which type the value is. This also implicitly impose a constraint on each type inherited from value.Kusto where the type must have a Convert method to do such conversion.

This code snippet may help you understand the idea: https://go.dev/play/p/j4rZWDJrdCu

@AsafMah
Copy link
Contributor

AsafMah commented Feb 15, 2022

Got it.

First of all, can you merge from main? there had been some changes and I want to resolve the conflicts early.

For now, can you expand the test you wrote to cover all types, just to make sure we didn't miss anything.

I also think we should check ExtractValue in etoe tests, I might add it myself to this PR.

@AsafMah AsafMah merged commit ca7cd57 into Azure:master Feb 21, 2022
@w1ndy w1ndy deleted the patch-extract-values branch February 21, 2022 08:49
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