kevingurney commented on code in PR #36978:
URL: https://github.com/apache/arrow/pull/36978#discussion_r1283286709
##########
matlab/src/matlab/+arrow/+array/Array.m:
##########
@@ -31,8 +31,11 @@
end
methods
- function obj = Array(varargin)
- obj.Proxy = libmexclass.proxy.Proxy(varargin{:});
+ function obj = Array(proxy)
+ arguments
Review Comment:
Indent?
##########
matlab/src/matlab/+arrow/array.m:
##########
@@ -0,0 +1,67 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+function arrowArray = array(data, opts)
+ arguments
+ data
+ opts.InferNulls(1, 1) logical = true
+ opts.Valid
+ end
+
+ data = convertCellstrToStrings(data);
Review Comment:
Maybe we should rename this to `convertCellstrToString`?
##########
matlab/src/matlab/+arrow/+array/TimestampArray.m:
##########
@@ -70,4 +62,26 @@
time(indices) = convertTo(dates(indices), "epochtime",
TicksPerSecond=ticksPerSecond(units));
end
end
+
+ methods(Static)
+ function array = fromMATLAB(data, opts)
+ arguments
+ data
+ opts.TimeUnit(1, 1) arrow.type.TimeUnit =
arrow.type.TimeUnit.Microsecond
+ opts.InferNulls(1, 1) logical = true
+ opts.Valid
+ end
+
+ arrow.internal.validate.type(data, "datetime");
+ arrow.internal.validate.shape(data);
+
+ validElements = arrow.internal.validate.parseValidElements(data,
opts);
+ ptime = arrow.array.TimestampArray.convertToEpochTime(data,
opts.TimeUnit);
Review Comment:
Maybe we should call this variable `epochTime`?
##########
matlab/test/arrow/array/tArray.m:
##########
@@ -0,0 +1,88 @@
+%TARRAY Unit tests for arrow.array function.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tArray < matlab.unittest.TestCase
+
+ properties(TestParameter)
+ MATLABDataArrayTypePair = { ...
+ {[true false], "arrow.array.BooleanArray"}, ...
+ {int8([1 2]), "arrow.array.Int8Array"}, ...
+ {uint8([1 2]), "arrow.array.UInt8Array"}, ...
+ {int16([1 2]), "arrow.array.Int16Array"}, ...
+ {uint16([1 2]), "arrow.array.UInt16Array"}, ...
+ {int32([1 2]), "arrow.array.Int32Array"}, ...
+ {uint32([1 2]), "arrow.array.UInt32Array"}, ...
+ {int64([1 2]), "arrow.array.Int64Array"}, ...
+ {uint64([1 2]), "arrow.array.UInt64Array"}, ...
+ {single([1 2]), "arrow.array.Float32Array"}, ...
+ {[1 2], "arrow.array.Float64Array"}, ...
+ {datetime(2022, 1, 1), "arrow.array.TimestampArray"}, ...
+ {["A" "B"], "arrow.array.StringArray"}};
+ end
+
+ methods(Test)
+ function ArrowArrayOutputType(testCase, MATLABDataArrayTypePair)
+ % Verify arrow.array returns the expected arrow.array.<Type>Array
+ % with respect to the input data array.
Review Comment:
"...with respect to the input data array's MATLAB class type."
##########
matlab/test/arrow/array/tTimestampArray.m:
##########
@@ -134,47 +133,54 @@ function TestInferNulls(testCase, TimeUnit, TimeZone)
end
function TestValidNVPair(testCase, TimeUnit, TimeZone)
- import arrow.array.TimestampArray
dates = datetime(2023, 6, 22, TimeZone=TimeZone) + days(0:4);
dates([2 4]) = NaT;
% Supply the Valid name-value pair as vector of indices.
- arrowArray = arrow.array.TimestampArray(dates, TimeUnit=TimeUnit,
Valid=[1 2 5]);
+ arrowArray = testCase.ArrowArrayConstructor(dates,
TimeUnit=TimeUnit, Valid=[1 2 5]);
testCase.verifyEqual(arrowArray.Valid, [true; true; false; false;
true]);
expectedDates = dates';
expectedDates(2) = getFillValue(TimeZone);
expectedDates([3 4]) = NaT;
testCase.verifyEqual(toMATLAB(arrowArray), expectedDates);
% Supply the Valid name-value pair as a logical scalar.
- arrowArray = arrow.array.TimestampArray(dates, TimeUnit=TimeUnit,
Valid=false);
+ arrowArray = testCase.ArrowArrayConstructor(dates,
TimeUnit=TimeUnit, Valid=false);
testCase.verifyEqual(arrowArray.Valid, [false; false; false;
false; false]);
expectedDates(:) = NaT;
testCase.verifyEqual(toMATLAB(arrowArray), expectedDates);
end
function ErrorIfNonVector(testCase)
- import arrow.array.TimestampArray
dates = datetime(2023, 6, 2) + days(0:11);
dates = reshape(dates, 2, 6);
- fcn = @() TimestampArray(dates);
- testCase.verifyError(fcn, "MATLAB:expectedVector");
+ fcn = @() testCase.ArrowArrayConstructor(dates);
+ testCase.verifyError(fcn, "arrow:array:InvalidShape");
dates = reshape(dates, 3, 2, 2);
- fcn = @() TimestampArray(dates);
- testCase.verifyError(fcn, "MATLAB:expectedVector");
+ fcn = @() testCase.ArrowArrayConstructor(dates);
+ testCase.verifyError(fcn, "arrow:array:InvalidShape");
end
function EmptyDatetimeVector(testCase)
import arrow.array.TimestampArray
dates = datetime.empty(0, 0);
- arrowArray = TimestampArray(dates);
+ arrowArray = testCase.ArrowArrayConstructor(dates);
+ testCase.verifyEqual(arrowArray.Length, int64(0));
+ testCase.verifyEqual(arrowArray.Valid, logical.empty(0, 1));
+ testCase.verifyEqual(toMATLAB(arrowArray), datetime.empty(0, 1));
+
+ % test with ND empty array
Review Comment:
ND -> NDimensional?
##########
matlab/test/arrow/internal/validate/tRealNumeric.m:
##########
@@ -0,0 +1,47 @@
+%TREALNUMERIC Unit tests for arrow.internal.validate.realnumeric.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tRealNumeric < matlab.unittest.TestCase
+
+ methods(TestClassSetup)
Review Comment:
I think this can be deleted.
##########
matlab/test/arrow/internal/validate/tShape.m:
##########
@@ -0,0 +1,69 @@
+%TREALNUMERIC Unit tests for arrow.internal.validate.shape.
Review Comment:
TREALNUMERIC -> TSHAPE
##########
matlab/src/matlab/+arrow/array.m:
##########
@@ -0,0 +1,67 @@
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+function arrowArray = array(data, opts)
+ arguments
+ data
+ opts.InferNulls(1, 1) logical = true
+ opts.Valid
+ end
+
+ data = convertCellstrToStrings(data);
+ classname = string(class(data));
+ args = namedargs2cell(opts);
+
+ switch (classname)
+ case "logical"
+ arrowArray = arrow.array.BooleanArray.fromMATLAB(data, args{:});
+ case "int8"
Review Comment:
This is a minor comment, but in a lot of other places we have been ordering
this list of types as `uint8`, `uint16`, ... rather than "interleaving" the
signed and unsigned integer types.
##########
matlab/test/arrow/array/tArray.m:
##########
@@ -0,0 +1,88 @@
+%TARRAY Unit tests for arrow.array function.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tArray < matlab.unittest.TestCase
+
+ properties(TestParameter)
+ MATLABDataArrayTypePair = { ...
+ {[true false], "arrow.array.BooleanArray"}, ...
+ {int8([1 2]), "arrow.array.Int8Array"}, ...
+ {uint8([1 2]), "arrow.array.UInt8Array"}, ...
+ {int16([1 2]), "arrow.array.Int16Array"}, ...
+ {uint16([1 2]), "arrow.array.UInt16Array"}, ...
+ {int32([1 2]), "arrow.array.Int32Array"}, ...
+ {uint32([1 2]), "arrow.array.UInt32Array"}, ...
+ {int64([1 2]), "arrow.array.Int64Array"}, ...
+ {uint64([1 2]), "arrow.array.UInt64Array"}, ...
+ {single([1 2]), "arrow.array.Float32Array"}, ...
+ {[1 2], "arrow.array.Float64Array"}, ...
+ {datetime(2022, 1, 1), "arrow.array.TimestampArray"}, ...
+ {["A" "B"], "arrow.array.StringArray"}};
+ end
+
+ methods(Test)
+ function ArrowArrayOutputType(testCase, MATLABDataArrayTypePair)
+ % Verify arrow.array returns the expected arrow.array.<Type>Array
+ % with respect to the input data array.
+ matlabArray = MATLABDataArrayTypePair{1};
+ expectedClassName = MATLABDataArrayTypePair{2};
+ arrowArray = arrow.array(matlabArray);
+ actualClassName = string(class(arrowArray));
+ testCase.verifyEqual(actualClassName, expectedClassName);
+ end
+
+ function UnsupportedMATLABTypeError(testCase)
+ % Verify arrow.array throws an error with the identifier
+ % "arrow:array:UnsupportedMATLABType" if the input array is not one
+ % we support converting into an Arrow array.
+ matlabArray = table;
+ fcn = @() arrow.array(matlabArray);
+ errID = "arrow:array:UnsupportedMATLABType";
+ testCase.verifyError(fcn, errID);
+ end
+
+ function InferNullsDefault(testCase)
+ % Verify InferNulls is true by default.
+ matlabArray = [1 2 NaN 3];
+ arrowArray = arrow.array(matlabArray);
+ testCase.verifyEqual(arrowArray.Valid, [true; true; false; true]);
+ end
+
+ function InferNullsTrue(testCase)
+ % Verify InferNulls is true by default.
Review Comment:
I think this should say something like "Verify that Valid contains the
expected logical vector when InferNulls=true."
##########
matlab/test/arrow/array/hNumericArray.m:
##########
@@ -92,20 +92,21 @@ function MaxValueTest(tc)
function ErrorIfComplex(tc)
fcn = @() tc.ArrowArrayConstructor(tc.MatlabArrayFcn([10 + 1i,
4]));
- tc.verifyError(fcn, "MATLAB:expectedReal");
+ tc.verifyError(fcn, "arrow:array:ComplexNumeric");
end
function ErrorIfNonVector(tc)
data = tc.MatlabArrayFcn([1 2 3 4 5 6 7 8 9]);
data = reshape(data, 3, 1, 3);
fcn = @() tc.ArrowArrayConstructor(tc.MatlabArrayFcn(data));
- tc.verifyError(fcn, "MATLAB:expectedVector");
+ tc.verifyError(fcn, "arrow:array:InvalidShape");
end
- function ErrorIfEmptyArrayIsNotTwoDimensional(tc)
+ function AllowNDEmptyArray(tc)
Review Comment:
`ND` -> `NDimensional`?
##########
matlab/test/arrow/array/tBooleanArray.m:
##########
@@ -18,7 +18,7 @@
properties
ArrowArrayClassName(1, 1) string = "arrow.array.BooleanArray"
- ArrowArrayConstructor = @arrow.array.BooleanArray
+ ArrowArrayConstructor = @arrow.array.BooleanArray.fromMATLAB
Review Comment:
Since we are using a static "construction" function now, maybe we should
rename this property to ArrowArrayConstructor**Fcn**?
##########
matlab/test/arrow/array/tArray.m:
##########
@@ -0,0 +1,88 @@
+%TARRAY Unit tests for arrow.array function.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tArray < matlab.unittest.TestCase
+
+ properties(TestParameter)
+ MATLABDataArrayTypePair = { ...
+ {[true false], "arrow.array.BooleanArray"}, ...
+ {int8([1 2]), "arrow.array.Int8Array"}, ...
+ {uint8([1 2]), "arrow.array.UInt8Array"}, ...
+ {int16([1 2]), "arrow.array.Int16Array"}, ...
+ {uint16([1 2]), "arrow.array.UInt16Array"}, ...
+ {int32([1 2]), "arrow.array.Int32Array"}, ...
+ {uint32([1 2]), "arrow.array.UInt32Array"}, ...
+ {int64([1 2]), "arrow.array.Int64Array"}, ...
+ {uint64([1 2]), "arrow.array.UInt64Array"}, ...
+ {single([1 2]), "arrow.array.Float32Array"}, ...
+ {[1 2], "arrow.array.Float64Array"}, ...
+ {datetime(2022, 1, 1), "arrow.array.TimestampArray"}, ...
+ {["A" "B"], "arrow.array.StringArray"}};
+ end
+
+ methods(Test)
+ function ArrowArrayOutputType(testCase, MATLABDataArrayTypePair)
+ % Verify arrow.array returns the expected arrow.array.<Type>Array
+ % with respect to the input data array.
+ matlabArray = MATLABDataArrayTypePair{1};
+ expectedClassName = MATLABDataArrayTypePair{2};
+ arrowArray = arrow.array(matlabArray);
+ actualClassName = string(class(arrowArray));
+ testCase.verifyEqual(actualClassName, expectedClassName);
+ end
+
+ function UnsupportedMATLABTypeError(testCase)
+ % Verify arrow.array throws an error with the identifier
+ % "arrow:array:UnsupportedMATLABType" if the input array is not one
+ % we support converting into an Arrow array.
+ matlabArray = table;
+ fcn = @() arrow.array(matlabArray);
+ errID = "arrow:array:UnsupportedMATLABType";
+ testCase.verifyError(fcn, errID);
+ end
+
+ function InferNullsDefault(testCase)
+ % Verify InferNulls is true by default.
+ matlabArray = [1 2 NaN 3];
+ arrowArray = arrow.array(matlabArray);
+ testCase.verifyEqual(arrowArray.Valid, [true; true; false; true]);
+ end
+
+ function InferNullsTrue(testCase)
+ % Verify InferNulls is true by default.
+ matlabArray = [1 2 NaN 3];
+ arrowArray = arrow.array(matlabArray, InferNulls=true);
+ testCase.verifyEqual(arrowArray.Valid, [true; true; false; true]);
+ end
+
+ function InferNullsFalse(testCase)
+ % Verify the Valid property of the arrow array when
+ % InferNulls=false.
+ matlabArray = [1 2 NaN 3];
+ arrowArray = arrow.array(matlabArray, InferNulls=false);
+ testCase.verifyEqual(arrowArray.Valid, [true; true; true; true]);
+ end
+
+ function ValidNameValuePair(testCase)
+ % Verify the Valid property of the arrow array when the Valid
Review Comment:
"Verify the Valid property returns the expected logical vector..."
##########
matlab/test/arrow/array/tBooleanArray.m:
##########
@@ -136,19 +136,20 @@ function ErrorIfNonVector(tc)
data = tc.MatlabArrayFcn([true false true false true false true
false true]);
data = reshape(data, 3, 1, 3);
fcn = @() tc.ArrowArrayConstructor(tc.MatlabArrayFcn(data));
- tc.verifyError(fcn, "MATLAB:expectedVector");
+ tc.verifyError(fcn, "arrow:array:InvalidShape");
end
- function ErrorIfEmptyArrayIsNotTwoDimensional(tc)
- data = tc.MatlabArrayFcn(reshape(logical.empty(0, 0), [1 0 0]));
- fcn = @() tc.ArrowArrayConstructor(data);
- tc.verifyError(fcn, "MATLAB:expected2D");
+ function AllowNDEmptyArray(tc)
Review Comment:
ND -> NDimensional?
##########
matlab/test/arrow/internal/validate/tShape.m:
##########
@@ -0,0 +1,69 @@
+%TREALNUMERIC Unit tests for arrow.internal.validate.shape.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tShape < matlab.unittest.TestCase
+
+ methods(Test)
+ function ErrorIf2DMatrix(testCase)
Review Comment:
2D -> 2Dimensional?
##########
matlab/test/arrow/array/tStringArray.m:
##########
@@ -136,13 +136,14 @@ function ErrorIfNonVector(tc)
data = tc.MatlabArrayFcn(["A", "B", "A", "B", "A", "B", "A", "B",
"A"]);
data = reshape(data, 3, 1, 3);
fcn = @() tc.ArrowArrayConstructor(tc.MatlabArrayFcn(data));
- tc.verifyError(fcn, "MATLAB:expectedVector");
+ tc.verifyError(fcn, "arrow:array:InvalidShape");
end
- function ErrorIfEmptyArrayIsNotTwoDimensional(tc)
+ function AllowNDEmptyArray(tc)
Review Comment:
ND -> NDimensional?
##########
matlab/test/arrow/internal/validate/tRealNumeric.m:
##########
@@ -0,0 +1,47 @@
+%TREALNUMERIC Unit tests for arrow.internal.validate.realnumeric.
+
+% Licensed to the Apache Software Foundation (ASF) under one or more
+% contributor license agreements. See the NOTICE file distributed with
+% this work for additional information regarding copyright ownership.
+% The ASF licenses this file to you under the Apache License, Version
+% 2.0 (the "License"); you may not use this file except in compliance
+% with the License. You may obtain a copy of the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS,
+% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+% implied. See the License for the specific language governing
+% permissions and limitations under the License.
+
+classdef tRealNumeric < matlab.unittest.TestCase
+
+ methods(TestClassSetup)
+ % Shared setup for the entire test class
+ end
+
+ properties(TestParameter)
+ NumericType = struct(int8=@int8,...
Review Comment:
Could we change this to not be "interleaved"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
