kevingurney commented on code in PR #37150: URL: https://github.com/apache/arrow/pull/37150#discussion_r1293606250
########## matlab/src/matlab/+arrow/+internal/+validate/+index/numericOrString.m: ########## @@ -0,0 +1,29 @@ +%NUMERICORSTRING Validates index is a valid numeric or string index. + +% 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 index = numericOrString(index, numericIndexType) + index = convertCharsToStrings(index); + if isnumeric(index) + index = arrow.internal.validate.index.numeric(index, numericIndexType); Review Comment: It would help shorten these lines a bit if we `import arrow.internal.validate,*` and then just call `index.numeric(...)` and `index.string(...)`. ########## matlab/src/matlab/+arrow/+internal/+validate/+index/numeric.m: ########## @@ -0,0 +1,53 @@ +%NUMERIC Validates the numeric index value. + +% 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 index = numeric(index, intType) + + if ~isnumeric(index) + errid = "arrow:badSubscript:NonNumeric"; + msg = "Expected numeric index values."; + error(errid, msg); + end + + % Convert to full storage if sparse + if issparse(index) + index = full(index); + end + + % Ensure the output is a column vector + index = reshape(index, [], 1); + + if any(index < 1) + errid = "arrow:badSubscript:NonPositive"; + msg = "Numeric indices must be positive integers."; + error(errid, msg); + elseif any(floor(index) ~= index) || any(isinf(index)) + errid = "arrow:badSubscript:NonInteger"; + msg = "Numeric indices must be finite positive integers."; + error(errid, msg); + elseif ~isreal(index) + errid = "arrow:badSubscript:NonReal"; + msg = "Numeric indices must be real positive integers."; Review Comment: I think this might read slightly clearer as "positive real integers". ########## matlab/test/arrow/internal/validate/index/tNumeric.m: ########## @@ -0,0 +1,165 @@ +%TNUMERIC Unit tests for arrow.internal.validate.index.numeric + +% 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 tNumeric < matlab.unittest.TestCase + + methods (Test) + + function NonPositiveError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonPositive" if the index array provided + % has non-positive values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonPositive"; + + fcn = @() numeric(0, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(-1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 -1 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonIntegerError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array provided + % has non-integer values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonInteger"; + + fcn = @() numeric(1.1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(NaN, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(inf, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 1.2 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonRealError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array is Review Comment: "arrow:badSubscript:NonInteger" -> "arrow:badSubscript:NonReal" ########## matlab/test/arrow/internal/validate/index/tNumeric.m: ########## @@ -0,0 +1,165 @@ +%TNUMERIC Unit tests for arrow.internal.validate.index.numeric + +% 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 tNumeric < matlab.unittest.TestCase + + methods (Test) + + function NonPositiveError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonPositive" if the index array provided + % has non-positive values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonPositive"; + + fcn = @() numeric(0, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(-1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 -1 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonIntegerError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array provided + % has non-integer values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonInteger"; + + fcn = @() numeric(1.1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(NaN, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(inf, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 1.2 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonRealError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array is + % complex. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonReal"; + + fcn = @() numeric(1 + 1i, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 2 + 2i], "int32"); + testCase.verifyError(fcn, errid); + end + + function ExceedsIntMaxError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array provided Review Comment: NonInteger -> ExceedsIntMax ########## matlab/test/arrow/internal/validate/index/tString.m: ########## @@ -0,0 +1,121 @@ +%TSTRING Unit tests for arrow.internal.validate.index.string + +% 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 tString < matlab.unittest.TestCase + + + methods(Test) + + function MissingStringError(testCase) + % Verify string() throws an error whose idenitifier is + % "arrow:badSubscript:NonPositive" if the index array provided Review Comment: NonPositive -> MissingString ########## matlab/test/arrow/internal/validate/index/tString.m: ########## @@ -0,0 +1,121 @@ +%TSTRING Unit tests for arrow.internal.validate.index.string + +% 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 tString < matlab.unittest.TestCase + + + methods(Test) + + function MissingStringError(testCase) + % Verify string() throws an error whose idenitifier is + % "arrow:badSubscript:NonPositive" if the index array provided + % has mising string values. + + import arrow.internal.validate.* + + errid = "arrow:badSubscript:MissingString"; + + fcn = @() index.string(string(missing)); + testCase.verifyError(fcn, errid); + + fcn = @() index.string(["A" missing "B"]); + testCase.verifyError(fcn, errid); + end + + function ZeroLengthTextError(testCase) + % Verify string() throws an error whose idenitifier is + % "arrow:badSubscript:ZeroLengthText" if the index array + % provided has zero length text values. + + import arrow.internal.validate.* + + errid = "arrow:badSubscript:ZeroLengthText"; + + fcn = @() index.string(""); + testCase.verifyError(fcn, errid); + + fcn = @() index.string(["A" "" "B"]); + testCase.verifyError(fcn, errid); + end + + function ValidStringIndices(testCase) + % Verify string() returns the expected string array if given + % a valid string, char, or cellstr as the index array. + + import arrow.internal.validate.* + + idx = index.string("A"); + testCase.verifyEqual(idx, "A"); + + idx = index.string(["A", "B"]); + testCase.verifyEqual(idx, ["A"; "B"]); + + idx = index.string('ABC'); + testCase.verifyEqual(idx, "ABC"); + + idx = index.string(['ABC'; 'DEF']); + testCase.verifyEqual(idx, "ADBECF"); + + idx = index.string({'Var1'}); + testCase.verifyEqual(idx, "Var1"); + + idx = index.string({'Var1', 'A'}); + testCase.verifyEqual(idx, ["Var1"; "A"]); + end + + function ErrorIfNonString(testCase) + % Verify string() throws an error whose idenitifer is + % "arrow:badSubscript:NonString" if neither a string array, + % char array, or cellstr array was provided as the index. Review Comment: or -> nor ########## matlab/src/matlab/+arrow/+internal/+validate/+index/numericOrString.m: ########## @@ -0,0 +1,29 @@ +%NUMERICORSTRING Validates index is a valid numeric or string index. + +% 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 index = numericOrString(index, numericIndexType) + index = convertCharsToStrings(index); + if isnumeric(index) + index = arrow.internal.validate.index.numeric(index, numericIndexType); + elseif isstring(index) + index = arrow.internal.validate.index.string(index); + else + errid = "arrow:badSubscript:UnsupportedIndexType"; Review Comment: I think we have been going with the convention of using all lowercase error ID "packages". So, perhaps, this should be "badsubscript" instead of "badSubscript" for consistency. ########## matlab/src/matlab/+arrow/+internal/+validate/+index/string.m: ########## @@ -0,0 +1,38 @@ +%NUMERIC Validates the numeric index value. + +% 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 index = string(index) + + index = convertCharsToStrings(index); + + index = reshape(index, [], 1); + + if (~isstring(index)) Review Comment: I think we can remove the outer parentheses here. ########## matlab/test/arrow/internal/validate/index/tNumericOrString.m: ########## @@ -0,0 +1,64 @@ +%tNUMERICORSTRING Unit tests for +% arrow.internal.validate.index.numericOrString. + +% 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 tNumericOrString < matlab.unittest.TestCase + + methods (Test) + + function validNumericIndex(testCase) Review Comment: Maybe for consistency we should make the method names `UpperCamelCase` since `tNumeric` uses that convention? ########## matlab/src/matlab/+arrow/+internal/+validate/+index/numeric.m: ########## @@ -0,0 +1,53 @@ +%NUMERIC Validates the numeric index value. + +% 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 index = numeric(index, intType) Review Comment: It would be helpful to add a comment describing what the `intType` argument is used for. ########## matlab/src/matlab/+arrow/+internal/+validate/+index/string.m: ########## @@ -0,0 +1,38 @@ +%NUMERIC Validates the numeric index value. Review Comment: I think this should say "STRING Validates the string index value." ########## matlab/src/matlab/+arrow/+internal/+validate/+index/string.m: ########## @@ -0,0 +1,38 @@ +%NUMERIC Validates the numeric index value. + +% 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 index = string(index) + + index = convertCharsToStrings(index); + + index = reshape(index, [], 1); + + if (~isstring(index)) + errid = "arrow:badSubscript:NonString"; + msg = "Expected string index values."; + error(errid, msg); + end + + if any(ismissing(index)) + errid = "arrow:badSubscript:MissingString"; + msg = "String indices must be nonmissing"; + error(errid, msg); + elseif any(strlength(index) == 0) + errid = "arrow:badSubscript:ZeroLengthText"; + msg = "String indices must not be zero length text values."; Review Comment: Maybe we could say something like "String indices must contain at least one character." ########## matlab/test/arrow/internal/validate/index/tNumeric.m: ########## @@ -0,0 +1,165 @@ +%TNUMERIC Unit tests for arrow.internal.validate.index.numeric + +% 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 tNumeric < matlab.unittest.TestCase + + methods (Test) + + function NonPositiveError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonPositive" if the index array provided + % has non-positive values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonPositive"; + + fcn = @() numeric(0, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(-1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 -1 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonIntegerError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array provided + % has non-integer values. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonInteger"; + + fcn = @() numeric(1.1, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(NaN, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric(inf, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 1.2 2], "int32"); + testCase.verifyError(fcn, errid); + end + + function NonRealError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array is + % complex. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:NonReal"; + + fcn = @() numeric(1 + 1i, "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 2 + 2i], "int32"); + testCase.verifyError(fcn, errid); + end + + function ExceedsIntMaxError(testCase) + % Verify numeric() throws an error whose idenitifier is + % "arrow:badSubscript:NonInteger" if the index array provided + % has values that exceed the intmax of the intType provided. + + import arrow.internal.validate.index.numeric + + errid = "arrow:badSubscript:ExceedsIntMax"; + + fcn = @() numeric(flintmax("double"), "int32"); + testCase.verifyError(fcn, errid); + + fcn = @() numeric([1 flintmax("double")], "int32"); + testCase.verifyError(fcn, errid); + end + + function CastToIntType(testCase) + % Verify numeric() returns an index array of the provided + % intType if the index array is valid. + + import arrow.internal.validate.index.numeric + + original = [1 2 3 4]; + expected = int32(original)'; + actual = numeric(original, "int32"); + testCase.verifyEqual(actual, expected); + + original = uint32([1 2 3 4]); + expected = int64(original)'; + actual = numeric(original, "int64"); + testCase.verifyEqual(actual, expected); + end + + function ConvertSparseToFullStorage(testCase) + % Verify numeric() converts sparse index arrays into full + % storage arrays. + + import arrow.internal.validate.index.numeric + + original = sparse([1 2 3 4]); + expected = int32(full(original))'; + actual = numeric(original, "int32"); + testCase.verifyEqual(actual, expected); + end + + function ErrorIfNotNumeric(testCase) Review Comment: ErrorIfNotNumeric -> ErrorIfNonNumeric -- 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]
