Uh oh!
There was an error while loading. Please reload this page.
GH-41246: [Docs][C++][Python] Improve docs on column encryption for nested fields - #45411
Conversation
543bd7b to
56e803dCompare| encryption_config->column_keys = "column_key_name: " | ||
| "ListColumn.list.element, " | ||
| "MapColumn.key_value.key, MapColumn.key_value.value, " | ||
| "StructColumn.f1, StructColumn.f2" |
There was a problem hiding this comment.
Are the spaces embedded in the string actually supported? Also, it seems to lack a semicolon at the end of the line.
There was a problem hiding this comment.
Yes it supports extra whitespaces, it trims the strings. The final semicolon is not needed, it would introduce a next empty section:
arrow/cpp/src/parquet/encryption/crypto_factory.cc
Lines 101 to 167 in 240ebc9
| An example for writing a dataset using encrypted Parquet file format: | ||
| .. code-block:: cpp |
There was a problem hiding this comment.
@jorisvandenbossche@AlenkaF@raulcd What is our preferred policy for code examples? Do we put them inline in the docs? Do we use separate files?
There was a problem hiding this comment.
Update: it seems we use literalinclude directives from C++ example files that are compiled as part of CI runs. See for example https://github.com/apache/arrow/blob/main/docs/source/cpp/dataset.rst#reading-datasets
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
EnricoMi
commented
Feb 7, 2025
There is some improvement for this non-intuitive naming scheme: #45462 |
pitrou
left a comment
There was a problem hiding this comment.
This LGTM except that we probably want to live in a separate file (see comment below).
6e27abf to
30c6184CompareEnricoMi
commented
Feb 14, 2025
@pitrou moved the C++ code into a file and referenced that from |
EnricoMi
commented
Mar 7, 2025
@pitrou addressed your comments, can this docs improvement be merged? |
pitrou
commented
Mar 7, 2025
The CI failures seem unexpected, can you rebase/merge from latest git main? |
EnricoMi
commented
Mar 7, 2025
Rebased with latest main commit d88ef57. |
pitrou
commented
Mar 7, 2025
Ok, the CI failures are certainly unrelated. |
pitrou
commented
Mar 7, 2025
@github-actions crossbow submit docs-preview |
|
AlenkaF
commented
Mar 18, 2025
@github-actions crossbow submit preview-docs |
Revision: 52fb219 Submitted crossbow builds: ursacomputing/crossbow @ actions-3c034c02a1
|
AlenkaF
left a comment
There was a problem hiding this comment.
Thank you for the updates! Here are the rendered files:
- http://crossbow.voltrondata.com/pr_docs/45411/cpp/parquet.html#encryption
- http://crossbow.voltrondata.com/pr_docs/45411/cpp/examples/parquet_column_encryption.html
- http://crossbow.voltrondata.com/pr_docs/45411/python/parquet.html#encryption-configuration
Looks good to me. Also the CI failures are not related.
EnricoMi
commented
Mar 24, 2025
@pitrou are you happy with this? |
Uh oh!
There was an error while loading. Please reload this page.
| Encrypting columns that have nested fields (struct, map, or even list data types) | ||
| requires column keys for the inner fields, not the column itself. | ||
| Configuring a column key for the column itself causes this error (here column name is ``col``): |
There was a problem hiding this comment.
| Encrypting columns that have nested fields (struct, map, or even list data types) | |
| requires column keys for the inner fields, not the column itself. | |
| Configuring a column key for the column itself causes this error (here column name is ``col``): | |
| Encrypting columns that have nested fields (struct, map or list data types) | |
| requires column keys for the inner fields, not the nested column itself. | |
| Configuring a column key for the nested column causes | |
| this error (here the column name is ``col``): |
There was a problem hiding this comment.
I find nested column ambiguous, it sounds like the inner column, the nested field.
What about outer column instead of nested column?
| Encrypting columns that have nested fields (struct, map, or even list data types) | |
| requires column keys for the inner fields, not the column itself. | |
| Configuring a column key for the column itself causes this error (here column name is ``col``): | |
| Encrypting columns that have nested fields (struct, map, or even list data types) | |
| requires column keys for the inner fields, not the outer column itself. | |
| Configuring a column key for the outer column causes this error (here column name is ``col``): |
There was a problem hiding this comment.
Also two wording nits:
- "struct, map, or even list data types" -> "struct, map, or list data types" (there's nothing special about lists)
- "here column name is" -> "here the column name is"
There was a problem hiding this comment.
Sorry, missed those of your changes when creating my suggestion. All incorporated now.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| ) | ||
| .. note:: | ||
| Encrypting columns that have nested fields (for instance struct, map, or even list data types) |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| #include <arrow/util/logging.h> |
There was a problem hiding this comment.
IMHO We should avoid using this header in public examples. std::cerr is good enough here.
| #include "arrow/result.h" | ||
| #include "parquet/arrow/reader.h" | ||
| #include <arrow/filesystem/path_util.h> |
There was a problem hiding this comment.
Can you move this include together with other Arrow includes above?
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
EnricoMi
commented
Mar 25, 2025
@pitrou all comments addressed, one alternative suggested at #45411 (comment) |
| #include "arrow/api.h" | ||
| #include "arrow/dataset/file_parquet.h" | ||
| #include "arrow/dataset/parquet_encryption_config.h" | ||
| #include "arrow/filesystem//localfs.h" |
There was a problem hiding this comment.
Small nit
| #include"arrow/filesystem//localfs.h" | |
| #include"arrow/filesystem/localfs.h" |
EnricoMi
commented
Mar 26, 2025
@pitrou comments addressed |
pitrou
commented
Mar 26, 2025
@github-actions crossbow submit preview-docs |
Revision: b47168d Submitted crossbow builds: ursacomputing/crossbow @ actions-ea617ad0a3
|
EnricoMi
commented
Mar 26, 2025
Thank you for your time, @pitrou! |
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 5d0149d. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
… for nested fields (apache#45411) ### Rationale for this change Encrypting columns with nested fields with a column key is not trivial since only leaf fields are allowed in the column key map. Documentation emphasizes this fact and provides examples. ### What changes are included in this PR? This amends the documentation on encryption for C++ and Python. ### Are these changes tested? Only documentation. ### Are there any user-facing changes? Only documentation. * GitHub Issue: apache#41246 Lead-authored-by: Enrico Minack <github@enrico.minack.dev> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
Rationale for this change
Encrypting columns with nested fields with a column key is not trivial since only leaf fields are allowed in the column key map. Documentation emphasizes this fact and provides examples.
What changes are included in this PR?
This amends the documentation on encryption for C++ and Python.
Are these changes tested?
Only documentation.
Are there any user-facing changes?
Only documentation.