Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
conbench-apache-arrow[bot] commented on PR #47014: URL: https://github.com/apache/arrow/pull/47014#issuecomment-3047039767 After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c4e201329e7ba78f8ca4a4d63e8c26ebc061b12e. There were 3 benchmark results indicating a performance regression: - Commit Run on `test-mac-arm` at [2025-07-07 21:09:46Z](https://conbench.ursa.dev/compare/runs/a8218fb9e2024d43b9acdbf756c76624...6a94feddbe47474994c220dbf5a72809/) - [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/0686c0014c5c7cc18659b465f88a...0686c43927d276cc8000788e1bb3ac7e) - [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-16, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/0686c00a26eb7a3480003daee3486e0e...0686c44281cd7fc48000d171d48c2148) - and 1 more (see the report linked below) The [full Conbench report](https://github.com/apache/arrow/runs/45519925547) has more details. It also includes information about 29 possible false positives for unstable benchmarks that are known to sometimes produce them. -- 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]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
sgilmore10 merged PR #47014: URL: https://github.com/apache/arrow/pull/47014 -- 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]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
sgilmore10 commented on PR #47014: URL: https://github.com/apache/arrow/pull/47014#issuecomment-3046137503 Addressed feedback. +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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
sgilmore10 commented on code in PR #47014:
URL: https://github.com/apache/arrow/pull/47014#discussion_r2190755585
##
matlab/src/matlab/+arrow/+tabular/Tabular.m:
##
@@ -0,0 +1,125 @@
+%TABULAR Interface for tabular data structure.
+
+% 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 Tabular < matlab.mixin.CustomDisplay & matlab.mixin.Scalar
+
+properties (Dependent, SetAccess=private, GetAccess=public)
+NumRows
+NumColumns
+ColumnNames
+Schema
+end
+
+properties (Hidden, SetAccess=private, GetAccess=public)
+Proxy
+end
+
+methods(Access=protected, Abstract)
+% constructColumnFromProxy must construct an instance of the
+% appropriate MATLAB class from the proxyInfo argument. The
+% template method arrow.tabular.Tabular/column() invokes this
+% method.
+column = constructColumnFromProxy(obj, proxyInfo)
+end
+
+methods
+function obj = Tabular(proxy)
+arguments
+proxy(1, 1) libmexclass.proxy.Proxy
+end
+import arrow.internal.proxy.validate
+obj.Proxy = proxy;
+end
+
+function numColumns = get.NumColumns(obj)
+numColumns = obj.Proxy.getNumColumns();
+end
+
+function numRows = get.NumRows(obj)
+numRows = obj.Proxy.getNumRows();
+end
+
+function columnNames = get.ColumnNames(obj)
+columnNames = obj.Proxy.getColumnNames();
+end
+
+function schema = get.Schema(obj)
+proxyID = obj.Proxy.getSchema();
+proxy = libmexclass.proxy.Proxy(Name="arrow.tabular.proxy.Schema",
ID=proxyID);
+schema = arrow.tabular.Schema(proxy);
+end
+
+function array = column(obj, idx)
+import arrow.internal.validate.*
+
+idx = index.numericOrString(idx, "int32", AllowNonScalar=false);
+
+if isnumeric(idx)
+args = struct(Index=idx);
+proxyInfo = obj.Proxy.getColumnByIndex(args);
+else
+args = struct(Name=idx);
+proxyInfo = obj.Proxy.getColumnByName(args);
+end
+
+array = obj.constructColumnFromProxy(proxyInfo);
+end
+
+function T = table(obj)
+import arrow.tabular.internal.*
+
+numColumns = obj.NumColumns;
+matlabArrays = cell(1, numColumns);
+
+for ii = 1:numColumns
+matlabArrays{ii} = toMATLAB(obj.column(ii));
+end
+
+validVariableNames = makeValidVariableNames(obj.ColumnNames);
+validDimensionNames = makeValidDimensionNames(validVariableNames);
+
+T = table(matlabArrays{:}, ...
+VariableNames=validVariableNames, ...
+DimensionNames=validDimensionNames);
+end
+
+function T = toMATLAB(obj)
+T = obj.table();
+end
+
+function tf = isequal(obj, varargin)
+tf = arrow.tabular.internal.isequal(obj, varargin{:});
+end
+
+end
+
+methods (Access = private)
+function str = toString(obj)
Review Comment:
done.
--
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]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
kevingurney commented on code in PR #47014: URL: https://github.com/apache/arrow/pull/47014#discussion_r2190463937 ## matlab/src/matlab/+arrow/+tabular/Tabular.m: ## @@ -0,0 +1,125 @@ +%TABULAR Interface for tabular data structure. Review Comment: ```suggestion %TABULAR Interface that represents a tabular data structure. ``` -- 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]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
kevingurney commented on code in PR #47014:
URL: https://github.com/apache/arrow/pull/47014#discussion_r2190456619
##
matlab/src/matlab/+arrow/+tabular/Tabular.m:
##
@@ -0,0 +1,125 @@
+%TABULAR Interface for tabular data structure.
+
+% 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 Tabular < matlab.mixin.CustomDisplay & matlab.mixin.Scalar
+
+properties (Dependent, SetAccess=private, GetAccess=public)
+NumRows
+NumColumns
+ColumnNames
+Schema
+end
+
+properties (Hidden, SetAccess=private, GetAccess=public)
+Proxy
+end
+
+methods(Access=protected, Abstract)
+% constructColumnFromProxy must construct an instance of the
+% appropriate MATLAB class from the proxyInfo argument. The
+% template method arrow.tabular.Tabular/column() invokes this
+% method.
+column = constructColumnFromProxy(obj, proxyInfo)
+end
+
+methods
+function obj = Tabular(proxy)
+arguments
+proxy(1, 1) libmexclass.proxy.Proxy
+end
+import arrow.internal.proxy.validate
+obj.Proxy = proxy;
+end
+
+function numColumns = get.NumColumns(obj)
+numColumns = obj.Proxy.getNumColumns();
+end
+
+function numRows = get.NumRows(obj)
+numRows = obj.Proxy.getNumRows();
+end
+
+function columnNames = get.ColumnNames(obj)
+columnNames = obj.Proxy.getColumnNames();
+end
+
+function schema = get.Schema(obj)
+proxyID = obj.Proxy.getSchema();
+proxy = libmexclass.proxy.Proxy(Name="arrow.tabular.proxy.Schema",
ID=proxyID);
+schema = arrow.tabular.Schema(proxy);
+end
+
+function array = column(obj, idx)
+import arrow.internal.validate.*
+
+idx = index.numericOrString(idx, "int32", AllowNonScalar=false);
+
+if isnumeric(idx)
+args = struct(Index=idx);
+proxyInfo = obj.Proxy.getColumnByIndex(args);
+else
+args = struct(Name=idx);
+proxyInfo = obj.Proxy.getColumnByName(args);
+end
+
+array = obj.constructColumnFromProxy(proxyInfo);
+end
+
+function T = table(obj)
+import arrow.tabular.internal.*
+
+numColumns = obj.NumColumns;
+matlabArrays = cell(1, numColumns);
+
+for ii = 1:numColumns
+matlabArrays{ii} = toMATLAB(obj.column(ii));
+end
+
+validVariableNames = makeValidVariableNames(obj.ColumnNames);
+validDimensionNames = makeValidDimensionNames(validVariableNames);
+
+T = table(matlabArrays{:}, ...
+VariableNames=validVariableNames, ...
+DimensionNames=validDimensionNames);
+end
+
+function T = toMATLAB(obj)
+T = obj.table();
+end
+
+function tf = isequal(obj, varargin)
+tf = arrow.tabular.internal.isequal(obj, varargin{:});
+end
+
+end
+
+methods (Access = private)
+function str = toString(obj)
Review Comment:
This is a nitpick, but, maybe we should add a newline here so that the
spacing is "balanced" inside of the `methods` block?
--
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]
Re: [PR] GH-38214: [MATLAB] Add a common `arrow.tabular.Tabular` MATLAB interface [arrow]
kevingurney commented on code in PR #47014:
URL: https://github.com/apache/arrow/pull/47014#discussion_r2190456619
##
matlab/src/matlab/+arrow/+tabular/Tabular.m:
##
@@ -0,0 +1,125 @@
+%TABULAR Interface for tabular data structure.
+
+% 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 Tabular < matlab.mixin.CustomDisplay & matlab.mixin.Scalar
+
+properties (Dependent, SetAccess=private, GetAccess=public)
+NumRows
+NumColumns
+ColumnNames
+Schema
+end
+
+properties (Hidden, SetAccess=private, GetAccess=public)
+Proxy
+end
+
+methods(Access=protected, Abstract)
+% constructColumnFromProxy must construct an instance of the
+% appropriate MATLAB class from the proxyInfo argument. The
+% template method arrow.tabular.Tabular/column() invokes this
+% method.
+column = constructColumnFromProxy(obj, proxyInfo)
+end
+
+methods
+function obj = Tabular(proxy)
+arguments
+proxy(1, 1) libmexclass.proxy.Proxy
+end
+import arrow.internal.proxy.validate
+obj.Proxy = proxy;
+end
+
+function numColumns = get.NumColumns(obj)
+numColumns = obj.Proxy.getNumColumns();
+end
+
+function numRows = get.NumRows(obj)
+numRows = obj.Proxy.getNumRows();
+end
+
+function columnNames = get.ColumnNames(obj)
+columnNames = obj.Proxy.getColumnNames();
+end
+
+function schema = get.Schema(obj)
+proxyID = obj.Proxy.getSchema();
+proxy = libmexclass.proxy.Proxy(Name="arrow.tabular.proxy.Schema",
ID=proxyID);
+schema = arrow.tabular.Schema(proxy);
+end
+
+function array = column(obj, idx)
+import arrow.internal.validate.*
+
+idx = index.numericOrString(idx, "int32", AllowNonScalar=false);
+
+if isnumeric(idx)
+args = struct(Index=idx);
+proxyInfo = obj.Proxy.getColumnByIndex(args);
+else
+args = struct(Name=idx);
+proxyInfo = obj.Proxy.getColumnByName(args);
+end
+
+array = obj.constructColumnFromProxy(proxyInfo);
+end
+
+function T = table(obj)
+import arrow.tabular.internal.*
+
+numColumns = obj.NumColumns;
+matlabArrays = cell(1, numColumns);
+
+for ii = 1:numColumns
+matlabArrays{ii} = toMATLAB(obj.column(ii));
+end
+
+validVariableNames = makeValidVariableNames(obj.ColumnNames);
+validDimensionNames = makeValidDimensionNames(validVariableNames);
+
+T = table(matlabArrays{:}, ...
+VariableNames=validVariableNames, ...
+DimensionNames=validDimensionNames);
+end
+
+function T = toMATLAB(obj)
+T = obj.table();
+end
+
+function tf = isequal(obj, varargin)
+tf = arrow.tabular.internal.isequal(obj, varargin{:});
+end
+
+end
+
+methods (Access = private)
+function str = toString(obj)
Review Comment:
Maybe we should add a newline here so that the spacing is "balanced" inside
of the `methods` block?
--
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]
