[HJwIMwqg] apoc.export.csv.graph incorrectly exports properties with datatype float #403
Conversation
| testCall(db, "CALL apoc.graph.fromDB('test',{}) yield graph " + | ||
| "CALL apoc.export.csv.graph(graph, $file,{useTypes: true, quotes: 'none'}) " + | ||
| "YIELD nodes, relationships, properties, file, source,format, time " + | ||
| "RETURN *", map("file", fileName), |
There was a problem hiding this comment.
Is this writing to the same resource file? If not, change the name of the file here so there's no confusion please
There was a problem hiding this comment.
changed the fileName value
| @@ -0,0 +1,80 @@ | |||
| package apoc.export.csv; | |||
There was a problem hiding this comment.
Why were these tests created in a separate file?
There was a problem hiding this comment.
Because there are extra nodes that would change the results of the ExportCsvTest test, so I created a new file to not make a lot of changes in that file and to simplify the assertions
| (r) -> assertResults(fileName, r, "graph", 2L, 1L, 16L, true)); | ||
| final String expected = Util.readResourceFile("manyTypes.csv"); | ||
| assertEquals(expected, readFile(fileName)); | ||
| } |
There was a problem hiding this comment.
Should we also have a test for apoc.export.csv.data?
| db.executeTransactionally("CREATE (n:SuperNode { one: datetime('2018-05-10T10:30[Europe/Berlin]'), two: time('18:02:33'), three: localtime('17:58:30'), \n" + | ||
| "four: localdatetime('2021-06-08'), five: date('2020'), six: duration({months: 5, days: 1.5}), seven : '2020'}) \n" + | ||
| "WITH n CREATE (n)-[:REL_TYPE {rel: point({x: 56.7, y: 12.78, crs: 'cartesian'})}]->(m:AnotherNode)"); |
There was a problem hiding this comment.
I'd include array properties here as well
There was a problem hiding this comment.
I think it's better not to include arrays at the moment
since there's another specific card for arrays (the one with id F9oIci9k),
so I could put them there in case
| } else { | ||
| result.addAll(Stream.of(starters).map(s -> s.split(":")[0]).collect(Collectors.toList())); | ||
| } | ||
| private List<Map.Entry<String, String>> generateHeader(Map<String, Class> propTypes, boolean useTypes, String... starters) { |
There was a problem hiding this comment.
Any reason why you have changed the type to List<Map.Entry<String, String>?
There was a problem hiding this comment.
Because this way I have a list of entries <PropertyName, PropertyDataType>,
which I think is easier to handle in later lines,
i.e. the writeNext() on line 170 (where I use both PropertyName and PropertyDataType )
and in the getNamesHeader(..), where I use PropertyName only, without needing to split the ":" to retrieve the names of the properties
| writeRels(graph, out, reporter, getNamesHeader(relHeader, REL_HEADER_FIXED_COLUMNS.length), cols, nodeHeader.size(), config.getBatchSize()); | ||
| } | ||
|
|
||
| private List<String> getNamesHeader(List<Map.Entry<String, String>> nodeHeader, int length) { |
There was a problem hiding this comment.
This is not always a nodeHeader so the name is misleading
There was a problem hiding this comment.
- It'll be good to include explanations of what is being done, the code of certain places looks very convoluted without any comment explaining what is doing.
- This PR requires more test coverage to be absolutely sure it's not breaking existing functionality and the new functionality works as expected.
Added several additional tests and some comments. |
Added data types with config
useTypes:true,consistently with these ones