Skip to content

add support for scan op - #3

Merged
jycor merged 4 commits into
masterfrom
james/scan
May 23, 2023
Merged

add support for scan op#3
jycor merged 4 commits into
masterfrom
james/scan

Conversation

@jycor

Copy link
Copy Markdown

The asterisk operator (*), is compiled into the "scan" operation, which we are missing a case for.
Fix for: dolthub/dolt#6006

@jycor
jycor requested a review from reltukMay 23, 2023 21:21

@timsehntimsehn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bad error message too

Comment threadjsonpath.go Outdated
return nil, err
}
default:
return nil, fmt.Errorf("expression don't support in filter")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This error sux. Fix that too.

@reltukreltuk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. A couple suggestions.

Comment threadjsonpath_test.go Outdated
"key": 1,
}
res, err := get_scan(obj)
fmt.Println(err, res)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file seems to use both fmt.Println and t.Logf in the tests, but t.Logf is preferable, so let's use that here.

Comment threadjsonpath.go Outdated
Comment on lines +571 to +573
for _, kv := range reflect.ValueOf(obj).MapKeys() {
res = append(res, reflect.ValueOf(obj).MapIndex(kv).Interface())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Slightly better as:

Suggested change
for_, kv:=rangereflect.ValueOf(obj).MapKeys() {
res=append(res, reflect.ValueOf(obj).MapIndex(kv).Interface())
}
iter:=reflect.ValueOf(obj).MapRange()
foriter.Next() {
res=append(res, iter.Value().Interface())
}

?

@jycor
jycor merged commit 77b8157 into masterMay 23, 2023
Sign up for freeto 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

@jycor@reltuk@timsehn