Till Westmann has posted comments on this change. Change subject: ASTERIXDB-976: CSV support for all basic types ......................................................................
Patch Set 2: Code-Review+1 (7 comments) Generally looks good to me. One thing that I noted is that there are a lot of magic constants in the code. But I realize that fixing that might be a bigger endeavor and that we might not want to block this change on this. https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ACirclePrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ACirclePrinter.java: Line 44: ps.print(" ]\""); Do 1, 9, and 17 have a meaning that we can express in a constant name? Or is a constant already available somewhere? https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ALinePrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ALinePrinter.java: Line 46: ps.print("] ]\""); Are there constants/could we create them for 1, 9, 17, 25? https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APoint3DPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APoint3DPrinter.java: Line 44: ps.print("]\""); Same question here. https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APointPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APointPrinter.java: Line 42: ps.print("]\""); And here. https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APolygonPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/APolygonPrinter.java: Line 57: ps.print(" ]\""); And here. https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ARectanglePrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/ARectanglePrinter.java: Line 47: ps.print("] ]\""); And here. https://asterix-gerrit.ics.uci.edu/#/c/444/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/AUUIDPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/AUUIDPrinter.java: Line 42: long lsb = LongPointable.getLong(b, s + 9); And here. -- To view, visit https://asterix-gerrit.ics.uci.edu/444 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7a515efd2bbf25895537413b45eb0992484c7412 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Chris Hillery <[email protected]> Gerrit-Reviewer: Chris Hillery <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
