You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kevingurney (via GitHub)" <gi...@apache.org> on 2023/10/10 20:02:26 UTC
[PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
kevingurney opened a new pull request, #38189:
URL: https://github.com/apache/arrow/pull/38189
### Rationale for this change
In support of adding an [`arrow.array.ListArray`](https://github.com/apache/arrow/issues/37815) MATLAB class, this pull request adds a new `arrow.type.ListType` MATLAB class.
### What changes are included in this PR?
1. New `arrow.list(<type>)` MATLAB construction function.
2. New `arrow.list.ListType` MATLAB class. `ListType` has a property named `Type` which indicates the inner type of the `List`. `Type` can be set to any subclass `arrow.type.Type` (including `arrow.type.ListType`, to support nested lists).
3. New `arrow.type.ID.List` type ID enumeration value.
4. New `arrow.type.traits.ListTraits` type traits class. Some of the properties, such as `ArrayConstructor` and `ArrayProxyClassName`, are set to `missing` because they are dependent on adding [`arrow.array.ListArray`](https://github.com/apache/arrow/issues/37815) first.
**Example**
```matlab
% Create a simple List<String> type.
>> stringListType = arrow.list(arrow.string())
stringListType =
ListType with properties:
ID: List
Type: [1x1 arrow.type.StringType]
% Create a nested List<List<Boolean>> type.
>> nestedListType = arrow.list(arrow.list(arrow.boolean()))
nestedListType =
ListType with properties:
ID: List
Type: [1x1 arrow.type.ListType]
% Extract the first-level, inner type, which is List<Boolean>.
>> innerType = nestedListType.Type
innerType =
ListType with properties:
ID: List
Type: [1x1 arrow.type.BooleanType]
% Extract the second-level, nested inner type, which is Boolean.
>> innerType.Type
ans =
BooleanType with properties:
ID: Boolean
```
### Are these changes tested?
Yes.
1. Added `tListType.m`.
2. Added `tListTraits.m`.
3. Updated `tField.m` to include `arrow.list`.
4. Updated `tID.m` to include `arrow.type.ID.List`.
6. Updated `tTypeDisplay.m` to include `arrow.type.ListType`.
7. Updated `ttraits.m` to include `arrow.type.traits.ListTraits`.
### Are there any user-facing changes?
Yes.
Client MATLAB code can now creates instances of `arrow.type.ListType` by using the `arrow.list(<type>)` construction function.
### Future Directions
1. #37815
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on PR #38189:
URL: https://github.com/apache/arrow/pull/38189#issuecomment-1757968763
+1
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353414070
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
+ % arguments is not an arrow.type.Type object.
+ fcn = @() testCase.ConstructionFcn("abc");
+ testCase.verifyError(fcn, "MATLAB:validation:UnableToConvert");
+ end
+
+ function ConstructionFcnEmptyTypeError(testCase)
+ % Verify construction function errors if given an empty
+ % arrow.type.Type array as one of its inputs.
Review Comment:
Thanks for catching this! This was a copy/paste mistake.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353727208
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.h:
##########
@@ -0,0 +1,36 @@
+// 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.
+
+#pragma once
+
+#include "arrow/matlab/type/proxy/type.h"
+
+namespace arrow::matlab::type::proxy {
+
+ class ListType : public arrow::matlab::type::proxy::Type {
+
+ public:
+ ListType(std::shared_ptr<arrow::ListType> list_type);
+
+ ~ListType() {}
+
+ void getType(libmexclass::proxy::method::Context& context);
+
+ static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments);
+};
Review Comment:
(We may want to enable code formatter like C++ does.)
```suggestion
};
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353413093
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
Review Comment:
Thanks for catching this! This was a copy/paste mistake.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355041217
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.h:
##########
@@ -0,0 +1,36 @@
+// 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.
+
+#pragma once
+
+#include "arrow/matlab/type/proxy/type.h"
+
+namespace arrow::matlab::type::proxy {
+
+ class ListType : public arrow::matlab::type::proxy::Type {
+
+ public:
+ ListType(std::shared_ptr<arrow::ListType> list_type);
+
+ ~ListType() {}
+
+ void getType(libmexclass::proxy::method::Context& context);
+
+ static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments);
+};
Review Comment:
That's a great point. I will create an issue for enabling `clang-format` checks in the MATLAB interface.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355061820
##########
matlab/src/matlab/+arrow/+type/ID.m:
##########
@@ -41,7 +41,7 @@
% IntervalDayTime (22)
% Decimal128 (23)
% Decimal256 (24)
- % List (25)
+ List (25)
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney merged PR #38189:
URL: https://github.com/apache/arrow/pull/38189
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355175852
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.cc:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+#include "arrow/matlab/type/proxy/list_type.h"
+#include "arrow/matlab/type/proxy/wrap.h"
+#include "libmexclass/proxy/ProxyManager.h"
+#include "arrow/matlab/error/error.h"
+
+namespace arrow::matlab::type::proxy {
+
+ ListType::ListType(std::shared_ptr<arrow::ListType> list_type) : Type(std::move(list_type)) {
+ REGISTER_METHOD(ListType, getType);
+ }
+
+ void ListType::getType(libmexclass::proxy::method::Context& context) {
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355067557
##########
matlab/src/matlab/+arrow/list.m:
##########
@@ -0,0 +1,26 @@
+% 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 l = list(type)
+%LIST Creates an arrow.type.ListType object
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353410474
##########
matlab/src/matlab/+arrow/+type/ID.m:
##########
@@ -41,7 +41,7 @@
% IntervalDayTime (22)
% Decimal128 (23)
% Decimal256 (24)
- % List (25)
+ List (25)
Review Comment:
Sure. I think it makes sense to just align all of the values to the longest type name.
##########
matlab/src/matlab/+arrow/list.m:
##########
@@ -0,0 +1,26 @@
+% 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 l = list(type)
+%LIST Creates an arrow.type.ListType object
Review Comment:
Good catch!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355007860
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.cc:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+#include "arrow/matlab/type/proxy/list_type.h"
+#include "arrow/matlab/type/proxy/wrap.h"
+#include "libmexclass/proxy/ProxyManager.h"
+#include "arrow/matlab/error/error.h"
+
+namespace arrow::matlab::type::proxy {
+
+ ListType::ListType(std::shared_ptr<arrow::ListType> list_type) : Type(std::move(list_type)) {
+ REGISTER_METHOD(ListType, getType);
+ }
+
+ void ListType::getType(libmexclass::proxy::method::Context& context) {
Review Comment:
One reason to change the name `Type` to `ValueType` is that other types will need to have multiple "Type" properties. For example, `DictionaryType`, will need to have two "Type" properties: `IndexType` and a`ValueType`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355203921
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.h:
##########
@@ -0,0 +1,36 @@
+// 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.
+
+#pragma once
+
+#include "arrow/matlab/type/proxy/type.h"
+
+namespace arrow::matlab::type::proxy {
+
+ class ListType : public arrow::matlab::type::proxy::Type {
+
+ public:
+ ListType(std::shared_ptr<arrow::ListType> list_type);
+
+ ~ListType() {}
+
+ void getType(libmexclass::proxy::method::Context& context);
+
+ static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments);
+};
Review Comment:
Captured idea of adding C++ linting checks in #38208.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355200665
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.h:
##########
@@ -0,0 +1,36 @@
+// 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.
+
+#pragma once
+
+#include "arrow/matlab/type/proxy/type.h"
+
+namespace arrow::matlab::type::proxy {
+
+ class ListType : public arrow::matlab::type::proxy::Type {
+
+ public:
+ ListType(std::shared_ptr<arrow::ListType> list_type);
+
+ ~ListType() {}
+
+ void getType(libmexclass::proxy::method::Context& context);
+
+ static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments);
+};
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353427672
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.cc:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+#include "arrow/matlab/type/proxy/list_type.h"
+#include "arrow/matlab/type/proxy/wrap.h"
+#include "libmexclass/proxy/ProxyManager.h"
+#include "arrow/matlab/error/error.h"
+
+namespace arrow::matlab::type::proxy {
+
+ ListType::ListType(std::shared_ptr<arrow::ListType> list_type) : Type(std::move(list_type)) {
+ REGISTER_METHOD(ListType, getType);
+ }
+
+ void ListType::getType(libmexclass::proxy::method::Context& context) {
Review Comment:
Thanks for bringing this up. I can certainly understand the concern about potential vagueness with the name `Type`. I am still considering the pros/cons of the different names.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355040320
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.cc:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+#include "arrow/matlab/type/proxy/list_type.h"
+#include "arrow/matlab/type/proxy/wrap.h"
+#include "libmexclass/proxy/ProxyManager.h"
+#include "arrow/matlab/error/error.h"
+
+namespace arrow::matlab::type::proxy {
+
+ ListType::ListType(std::shared_ptr<arrow::ListType> list_type) : Type(std::move(list_type)) {
+ REGISTER_METHOD(ListType, getType);
+ }
+
+ void ListType::getType(libmexclass::proxy::method::Context& context) {
Review Comment:
That's an excellent point! Given that consideration, I agree it makes sense to change the name to `ValueType`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "sgilmore10 (via GitHub)" <gi...@apache.org>.
sgilmore10 commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1353282288
##########
matlab/src/cpp/arrow/matlab/type/proxy/list_type.cc:
##########
@@ -0,0 +1,64 @@
+// 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.
+
+#include "arrow/matlab/type/proxy/list_type.h"
+#include "arrow/matlab/type/proxy/wrap.h"
+#include "libmexclass/proxy/ProxyManager.h"
+#include "arrow/matlab/error/error.h"
+
+namespace arrow::matlab::type::proxy {
+
+ ListType::ListType(std::shared_ptr<arrow::ListType> list_type) : Type(std::move(list_type)) {
+ REGISTER_METHOD(ListType, getType);
+ }
+
+ void ListType::getType(libmexclass::proxy::method::Context& context) {
Review Comment:
I wonder if we should name the `Type` property in `ListType` to `ValueType` or `Value`. However, I'm ok with `Type` for now. I just wonder if the name is too vague.
##########
matlab/src/matlab/+arrow/+type/ID.m:
##########
@@ -41,7 +41,7 @@
% IntervalDayTime (22)
% Decimal128 (23)
% Decimal256 (24)
- % List (25)
+ List (25)
Review Comment:
Could you align `(25)` and `(26)`?
##########
matlab/src/matlab/+arrow/list.m:
##########
@@ -0,0 +1,26 @@
+% 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 l = list(type)
+%LIST Creates an arrow.type.ListType object
Review Comment:
Can you move the help text above the license header?
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
+ % arguments is not an arrow.type.Type object.
+ fcn = @() testCase.ConstructionFcn("abc");
+ testCase.verifyError(fcn, "MATLAB:validation:UnableToConvert");
+ end
+
+ function ConstructionFcnEmptyTypeError(testCase)
+ % Verify construction function errors if given an empty
+ % arrow.type.Type array as one of its inputs.
Review Comment:
as one of its inputs -> as the input argument.
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
Review Comment:
if any one of the input arguments -> if the input argument is not
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "kevingurney (via GitHub)" <gi...@apache.org>.
kevingurney commented on code in PR #38189:
URL: https://github.com/apache/arrow/pull/38189#discussion_r1355064770
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
+ % arguments is not an arrow.type.Type object.
+ fcn = @() testCase.ConstructionFcn("abc");
+ testCase.verifyError(fcn, "MATLAB:validation:UnableToConvert");
+ end
+
+ function ConstructionFcnEmptyTypeError(testCase)
+ % Verify construction function errors if given an empty
+ % arrow.type.Type array as one of its inputs.
Review Comment:
Fixed.
##########
matlab/test/arrow/type/tListType.m:
##########
@@ -0,0 +1,124 @@
+% TLISTTYPE Tests for arrow.type.ListType
+
+% 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 tListType < matlab.unittest.TestCase
+
+ properties (Constant)
+ BasicList = arrow.list(arrow.int8())
+ NestedList = arrow.list(arrow.list(arrow.list(arrow.uint64())))
+ ConstructionFcn = @arrow.list
+ TypeID = arrow.type.ID.List
+ ClassName = "arrow.type.ListType"
+ end
+
+ methods (Test)
+
+ function ConstructionFcnBasic(testCase)
+ % Verify construction function returns an instance of the
+ % expected arrow.type.Type subclass.
+ type = testCase.BasicList;
+ testCase.verifyInstanceOf(type, testCase.ClassName);
+ end
+
+ function ConstructionFcnTooFewInputsError(testCase)
+ % Verify construction function errors if given zero input arguments.
+ fcn = @() testCase.ConstructionFcn();
+ testCase.verifyError(fcn, "MATLAB:minrhs");
+ end
+
+ function ConstructionFcnInvalidInputTypeError(testCase)
+ % Verify construction function errors if any one of the input
Review Comment:
Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37812: [MATLAB] Add `arrow.type.ListType` MATLAB class [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38189:
URL: https://github.com/apache/arrow/pull/38189#issuecomment-1763220111
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 852f09d54b4fb92b392ad7fa29364e6add13bd3b.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17708860438) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org