-
Notifications
You must be signed in to change notification settings - Fork 440
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
implement array slicing #106
Conversation
Just pushed support for $ jsonnet git:(slice) ✗ cat exp.jsonnet
local arr = std.range(0, 10);
{
foo: arr[5:7],
bar: arr[:7],
baz: arr[5:],
}
$ jsonnet git:(slice) ✗ ./jsonnet exp.jsonnet
{
"bar": [
0,
1,
2,
3,
4,
5,
6
],
"baz": [
5,
6,
7,
8,
9,
10
],
"foo": [
5,
6
]
} |
This doesn't support step parameter yet. |
0e77329
to
2ac596b
Compare
@@ -317,6 +318,15 @@ struct Index : public AST { | |||
{ } | |||
}; | |||
|
|||
struct Slice : public AST { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an advantage to having this as a separate AST if it can only be used inside the [ ] of an Index AST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically you could extend Index to add the last and step ASTs which are null if they're not provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Good work! Sorry it took me a few days to get around to reviewing it. |
@@ -88,6 +88,9 @@ limitations under the License. | |||
range(from, to):: | |||
std.makeArray(to - from + 1, function(i) i + from), | |||
|
|||
slice(arr, from, to):: | |||
std.makeArray(to - from, function(i) arr[i + from]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also work on strings and fail explicitly if arr is not a string or array type. There is already str.substr but its semantics aren't right (see comment below). So we should deprecate str.substr() and use this instead.
06b5eb7
to
de15396
Compare
Ok, I need to implement the python behavior in std.slice now. |
I've implemented python behavior for all positive value (index, end, step) inputs. I think it's reasonable to support negative inputs later on. |
//{ | ||
// input: arr[::], | ||
// output: arr, | ||
//}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sparkprime This is breaking with:
FAIL (exit code): slice.sugar.jsonnet
This run's output:
STATIC ERROR: slice.sugar.jsonnet:32:21-22: Not a unary operator: ::
I'll investigate what is going on but it's not immediately obvious to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is bad unfortunately -- { x:: 3 } is lexed as the single token ::
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can change the parser to check for that, a bit like C++ does with
C<vector<int>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A workaround is also to do [ : : ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed with https://github.com/mikedanese/jsonnet/blob/slice/core/parser.cpp#L1201-L1208 and enabled the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it without push() but it requires a whole nother branch which feels gross
if (ast->end != nullptr) { | ||
desugar(ast->end, obj_level); | ||
} else { | ||
ast->end = length(ast->target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you desugared them to null, you could test for null in std.slice and implement the full semantics that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> [1,2,3][None:None:None]
[1, 2, 3]
In fact this is mandatory for python compatibility :)
0196ebf
to
9754fa9
Compare
@sparkprime addressed all review comments. |
else | ||
slice + [ invar.indexable[cur] ], | ||
cur + invar.step | ||
) tailstrict; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O(n^2) :( but would rather optimize this in the vm for array concatinations where one side is not used again.
$ jsonnet git:(slice): cat exp.jsonnet
std.range(0, std.extVar("vars").count)[2:100000000000:2]
$ jsonnet git:(slice): time ./jsonnet --code-var vars='{"count": 1000}' exp.jsonnet > /dev/null
./jsonnet --code-var vars='{"count": 1000}' exp.jsonnet > /dev/null 0.25s user 0.00s system 99% cpu 0.258 total
$ jsonnet git:(slice): time ./jsonnet --code-var vars='{"count": 10000}' exp.jsonnet > /dev/null
./jsonnet --code-var vars='{"count": 10000}' exp.jsonnet > /dev/null 1.98s user 0.01s system 99% cpu 1.990 total
$ jsonnet git:(slice): time ./jsonnet --code-var vars='{"count": 100000}' exp.jsonnet > /dev/null
./jsonnet --code-var vars='{"count": 100000}' exp.jsonnet > /dev/null 46.23s user 0.30s system 99% cpu 46.594 total
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I am not sure how to solve this. There are various instances of it throughout the code.
The Python approach is to make std.join O(n) and then do everything in terms of that.
Currently std.join is O(n^2) and making it O(n) would mean making it native.
There may be a better way though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about implementing the internal representation with a persistent data structure. Arrays could still be copy on write minus the deep copy.
Incomplete pass at #101