zhztheplayer commented on a change in pull request #7030: URL: https://github.com/apache/arrow/pull/7030#discussion_r417034247
########## File path: cpp/src/jni/dataset/proto/Types.proto ########## @@ -0,0 +1,149 @@ +// 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. + +syntax = "proto2"; +package types; + +option java_package = "org.apache.arrow.dataset"; Review comment: @wesm > So if there are changes in the C++ API, then refactoring can take place only at the JNI C++ code and no changes will be required in Java. Sorry I am not quite getting it. Is it really needed to change Java code if c++ filter changed when it comes to the current PR? I mean, now the "layers" of the approach are something like: ``` Java: DatasetTypes.Condition (ProtoBuf generated code) | Java: byte[] | JNI Bridge | C++: uint8_t* | C++ types::Condition (ProtoBuf generated code) | <---- c++ function TranslateFilter(...) C++ arrow::dataset::Expression (Dataset Filters) ``` The top 5 layers are all based on protobuf generated codes so we don't have to change any of them from Java-side when we want to make changes from C++. Even say if we want to change Java filter API we can also just modify file Types.proto. Or in future if we refactor C++ API arrow::dataset::Expression, we may have to make related changes to the function `TranslateFilter(...)` as well to make JNI things matched but we don't have to change Java codes anyway. And the filter tree structures are not 1-1 mapped too, the definitions in Types.proto are much simpler than arrow::dataset::Expression so far, I didn't mean to make them strictly match. In future we can change C++ filters API without touching Java API and it is fine enough to me. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
